Fix/payara4003 Inaccessible constructor with @ConstructorProperties #280

JohT · 2019-07-03T14:50:23Z

Fixes payara/Payara#4003.
Inaccessible constructors annotated with @ConstructorProperties should be logged (finest) and ignored.
They shouldn't override a no-arg constructor (+setter).
They shouldn't be detected as JsonbCreator anyway, since they would lead to an IllegalAccessException later during deserialization.

@ConstructorProperties support had been introduced with
#224.

jakartaee/jsonb-api#115 (comment) Signed-off-by: Johannes <[email protected]>

…oved to ComponentMatcher. Signed-off-by: Roman Grigoriadi <[email protected]>

Added required module for ConstructorProperties-Annotation Signed-off-by: Johannes <[email protected]>

Signed-off-by: Johannes <[email protected]>

Merge changes from original repository Signed-off-by: Johannes <[email protected]>

Signed-off-by: Johannes <[email protected]>

Signed-off-by: Johannes <[email protected]> # Conflicts: # src/main/java/module-info.java # src/main/java/org/eclipse/yasson/internal/ConstructorPropertiesAnnotationIntrospector.java # src/main/java/org/eclipse/yasson/internal/properties/MessageKeys.java # src/main/resources/yasson-messages.properties

Signed-off-by: Johannes <[email protected]>

aguibert

Thanks for the PR @JohT! Just some minor review comments and then it should be good to merge.

aguibert · 2019-07-15T21:33:32Z

src/main/java/org/eclipse/yasson/internal/properties/MessageKeys.java

@@ -88,6 +88,7 @@
    MISSING_VALUE_PROPERTY_IN_ANNOTATION("missingValuePropertyInAnnotation"),
    NUMBER_INCOMPATIBLE_VALUE_TYPE_ARRAY("numberIncompatibleValueTypeArray"),
    NUMBER_INCOMPATIBLE_VALUE_TYPE_OBJECT("numberIncompatibleValueTypeObject"),
+    INACCESSIBLE_CONSTRUCTOR_PROPERTIES_CREATOR("inaccessibleConstructorPropertiesCreator"),


it's not necessary to use translated message keys for debug level messages -- only INFO and above need those

aguibert · 2019-07-15T21:54:45Z

src/test/java/org/eclipse/yasson/internal/AnnotationIntrospectorTestFixtures.java

@@ -31,7 +31,7 @@
        return AccessController.doPrivileged((PrivilegedAction<Constructor<?>[]>) clazz::getDeclaredConstructors);
    }

-    public static class ObjectWithoutAnnotatedConstructor implements ProvidesParameterRepresentation {


I didn't name these test objects originally, but I do find the original class names to be more clear. Can we please revert the names back?

I reverted them back. I named them originally and thought, they may get to long. But it is much cleaner for this branch to let it as it was.

aguibert · 2019-07-15T21:59:19Z

src/main/java/org/eclipse/yasson/internal/ConstructorPropertiesAnnotationIntrospector.java

@@ -43,6 +44,11 @@ public JsonbCreator getCreator(Constructor<?>[] constructors) {
            if (!(properties instanceof String[])) {
                continue;
            }
+            if (!Modifier.isPublic(constructor.getModifiers())) {
+                String declaringClass = constructor.getDeclaringClass().getName();
+                LOG.finest(Messages.getMessage(MessageKeys.INACCESSIBLE_CONSTRUCTOR_PROPERTIES_CREATOR, declaringClass, Arrays.toString((String[]) properties)));


Had a second thought on this one -- since this situation represents a static error in the code I think we should blow up with the message you've written. I see no reason to tolerate an extra annotation which will only ever be ignored by JSON-B

If "we should blow up" means that we should throw an exception, then this would not solve payara/Payara#4003.

I would agree with you, when it comes to value objects, that only have this one annotated but inaccessible constructor, and no other way to construct them. This also meets the spec.

BUT :-) If there is no explicitly annotated creator, then the "normal" no-arg constructor is used to create an instance (see ObjectDeserializer.getInstance()). This should work, regardless of any additional constructor, that is annotated with @ConstructorProperties.

Value Objects with:

public no-arg constructor

getter/setter or field access

one additional, non-public constructor annotated with @ConstructorProperties

will fail to get deserialized, only because of the @ConstructorProperties-support, that came with 1.0.4. (ok, that's on me :-))

One way to solve this is to log an inaccessible @ConstructorProperties annotated constructor and ignore it. There may be a no-arg constructor later and everything is fine. There may be no way to create an instance and the deserialization would fail a bit later than it should (not that nice indeed).

The other way would be to throw an exception, but only in the case, when there is no useable other way to create an instance. This would meet the "fail fast" principle, but will lead to do the "normal" detection earlier during getCreator() additional to ObjectDeserializer.getInstance(). This could get a bit complicated and eventually messy.

One interesting thing last but not least:
I added
"testNoArgConstructorShouldBePreferredOverUnusableJsonbAnnotatedProtectedConstructor" to "AnnotationIntrospectorTest". This test case is currently ignored, since it does not work as i'd expect. It shows, that the problem also applies to @JsonbCreator-annotated, inaccessible constructors with an additional valid no-arg constructor. The specification does not forbid the current behavior, as far as i've seen.

Both of these special cases shouldn't come up that often. But since there are generators and tools like lombok, value-objects may not always end up as they would in a hand written way.

I suppose we can just log a message and move on as you suggest, but since this is a FINEST log level, lets not use a translated message from the yasson-messages resource file

Signed-off-by: Johannes <[email protected]>

JohT · 2019-09-02T18:37:21Z

Is there anything I can do to help solving this?

aguibert

hi @JohT, the only remaining review comment is to remove the translated message from yasson-message.properties and MessageKeys.java.
Also, if you could open up a separate issue to track the ignored test that would be great.
Once you do these we can go ahead and merge

aguibert · 2019-09-03T04:34:44Z

src/main/java/org/eclipse/yasson/internal/ConstructorPropertiesAnnotationIntrospector.java

@@ -43,6 +44,11 @@ public JsonbCreator getCreator(Constructor<?>[] constructors) {
            if (!(properties instanceof String[])) {
                continue;
            }
+            if (!Modifier.isPublic(constructor.getModifiers())) {
+                String declaringClass = constructor.getDeclaringClass().getName();
+                LOG.finest(Messages.getMessage(MessageKeys.INACCESSIBLE_CONSTRUCTOR_PROPERTIES_CREATOR, declaringClass, Arrays.toString((String[]) properties)));


I suppose we can just log a message and move on as you suggest, but since this is a FINEST log level, lets not use a translated message from the yasson-messages resource file

aguibert · 2019-09-03T04:40:42Z

src/test/java/org/eclipse/yasson/internal/AnnotationIntrospectorTest.java

+        assertCreatedInstanceContainsAllParameters(ObjectWithJsonbCreatorAnnotatedProtectedConstructor.example(), creator);
+    }
+
+    // TODO Under discussion. Shouldn't this test work?


so currently this test fails with an IllegalAccessException? If so, please open up another issue and link to the URL in the comment here so we know why the test is ignored

done.
i opened #326.

aguibert · 2019-09-03T16:11:58Z

FYI @JohT I'm aiming to cut a 1.0.5 release of Yasson by the end of this week. If you can get the remaining review comments addressed by then we can include this PR in the 1.0.5 release

Signed-off-by: Johannes <[email protected]>

aguibert

looks good now. Thanks @JohT!

JohT and others added 12 commits February 11, 2019 20:19

Support for @ConstructorProperties as proposed in jsonb-api/issues/115

3e71c0c

jakartaee/jsonb-api#115 (comment) Signed-off-by: Johannes <[email protected]>

Added missing final keywords. Flag for generic serializers/adapters m…

4f0e825

…oved to ComponentMatcher. Signed-off-by: Roman Grigoriadi <[email protected]>

Update module-info.java

938b903

Added required module for ConstructorProperties-Annotation Signed-off-by: Johannes <[email protected]>

Update pom.xml

290ad17

Added required module for ConstructorProperties-Annotation Signed-off-by: Johannes <[email protected]>

formatting

0a14999

Signed-off-by: Johannes <[email protected]>

Merged changes from original repository

b4202c8

Signed-off-by: Johannes <[email protected]>

Support for @ConstructorProperties as proposed in jsonb-api/issues/115

c1495b4

Signed-off-by: Johannes <[email protected]>

Add builtin serializer for java.sql.Timestamp

3882ad9

Merge pull request #3 from eclipse-ee4j/master

c1b5441

Merge changes from original repository Signed-off-by: Johannes <[email protected]>

Further tests, reset formatting

87fe261

Signed-off-by: Johannes <[email protected]>

fix for payara/Payara#4003

9d089ce

Signed-off-by: Johannes <[email protected]>

JohT force-pushed the fix/payara4003 branch from bd2f279 to 9d089ce Compare July 3, 2019 18:36

aguibert reviewed Jul 15, 2019

View reviewed changes

JohT added 2 commits July 17, 2019 20:46

test fixture names reverted back

8e1161c

Signed-off-by: Johannes <[email protected]>

Added one currently ignored unit test for discussion

7bdc022

Signed-off-by: Johannes <[email protected]>

aguibert reviewed Sep 3, 2019

View reviewed changes

JohT added 2 commits September 4, 2019 21:26

Merge branch 'master' of https://github.com/eclipse-ee4j/yasson

88bf0eb

Signed-off-by: Johannes <[email protected]>

Merge branch 'master' into fix/payara4003

64c5f98

Signed-off-by: Johannes <[email protected]>

JohT mentioned this pull request Sep 4, 2019

@JsonbCreator-annotated inaccessible constructors shouldn't overrule no-arg constructor #326

Closed

removed message key for info log.

74a96f4

Signed-off-by: Johannes <[email protected]>

aguibert approved these changes Sep 5, 2019

View reviewed changes

aguibert merged commit 09d7a24 into eclipse-ee4j:master Sep 5, 2019

aguibert added the bug Something isn't working right label Sep 6, 2019

aguibert added this to the 1.0.5 milestone Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/payara4003 Inaccessible constructor with @ConstructorProperties #280

Fix/payara4003 Inaccessible constructor with @ConstructorProperties #280

JohT commented Jul 3, 2019

aguibert left a comment

aguibert Jul 15, 2019

aguibert Jul 15, 2019

JohT Jul 17, 2019

aguibert Jul 15, 2019

JohT Jul 17, 2019

aguibert Sep 3, 2019

JohT commented Sep 2, 2019

aguibert left a comment

aguibert Sep 3, 2019

aguibert Sep 3, 2019

JohT Sep 4, 2019

aguibert commented Sep 3, 2019

aguibert left a comment

Fix/payara4003 Inaccessible constructor with @ConstructorProperties #280

Fix/payara4003 Inaccessible constructor with @ConstructorProperties #280

Conversation

JohT commented Jul 3, 2019

aguibert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohT commented Sep 2, 2019

aguibert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aguibert commented Sep 3, 2019

aguibert left a comment

Choose a reason for hiding this comment