You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by GitBox <gi...@apache.org> on 2021/01/16 20:42:49 UTC
[GitHub] [johnzon] reta opened a new pull request #73: Fix InaccessibleObjectException on JDK16+
reta opened a new pull request #73:
URL: https://github.com/apache/johnzon/pull/73
Since JEP-396 [1] integration into JDK16, the JDK internals become much less accessible. `Johnzon` uses `setAccessible` in several places to relax the visibility rules but it now throws `InaccessibleObjectException` on JDK16 and above (for a number of JDK classes). For example, we run into following issues with Apache CXF while running JSON-B tests on JDK16/JDK17:
```
java.lang.reflect.InaccessibleObjectException: Unable to make field private int java.util.ArrayList.size accessible: module java.base does not "opens java.util" to unnamed module @567d299b
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldDecoratedType.<init>(FieldAccessMode.java:105)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldReader.<init>(FieldAccessMode.java:169)
at org.apache.johnzon.mapper.access.FieldAccessMode.doFindReaders(FieldAccessMode.java:49)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.mapper.access.FieldAndMethodAccessMode.doFindReaders(FieldAndMethodAccessMode.java:68)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.jsonb.JsonbAccessMode.findReaders(JsonbAccessMode.java:468)
at org.apache.johnzon.mapper.Mappings.createClassMapping(Mappings.java:475)
at org.apache.johnzon.mapper.Mappings.doFindOrCreateClassMapping(Mappings.java:437)
at org.apache.johnzon.mapper.Mappings.findOrCreateClassMapping(Mappings.java:411)
at org.apache.johnzon.mapper.Mapper.isDeduplicateObjects(Mapper.java:215)
at org.apache.johnzon.mapper.Mapper.writeObjectWithGenerator(Mapper.java:209)
at org.apache.johnzon.mapper.Mapper.writeObject(Mapper.java:203)
at org.apache.johnzon.mapper.Mapper.writeObject(Mapper.java:230)
```
The suggested fix in this PR is to ignore the properties which are not accessible anymore (basically when `setAccessible` ends up with `InaccessibleObjectException` exception). Alternatively, it is still possible to use `--add-opens java.base/java.util=ALL-UNNAMED` but arguably this not considered acceptable.
[1] https://openjdk.java.net/jeps/396
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau edited a comment on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau edited a comment on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766141778
@reta can you review https://issues.apache.org/jira/browse/JOHNZON-331 and the related commit (https://github.com/apache/johnzon/commit/15a4cc69344b3a0dd7ee80326cc064fa0972a779). Does it solve your issue or do we need "more" - note that it can need some more love from cxf side is type is unexpectedly erased like using getType instead of getGenericType somewhere maybe?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761643605
Hi @reta
Can you keep the public constructors please? It is used by extensions/custom codes.
Also wonder if:
1. Optional usage can be dropped
2. Accessibility can be tested
3. Accessibility can still be enforced (worse case adding a build time generation) - can help on that in ~7dayd
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559867922
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
Hey @rmannibucau, I gave it a second thought, particularly when you mentioned the presence of the extensions and completely refactored the approach to eliminate flows driven by exceptions. I think it is more clean but let us discuss it when you have time, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761814980
>
>
> Hi @reta, had a quick look. Seems it is only an issue if in a not yet opened module so we should be able yo skip fields in this case no - and not have to catch it as a fallback. Wdyt? Can help on code side in 7days.
It seems like a viable option as well, probably would need some reflection in order to use the APIs available in JDK9+, I could try it for sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766164766
@rmannibucau Mapping a not opened/imported model is certainly not advisable, probably the question would be: should `Johnzon` do best effort by not trying to access what it cannot, or just fail fast. Sadly I do not have the only right answer, but I think certain measures from the library/framework could help. Knowing that the semantic of the `setAccessible` has changed, try to access and skip if it is not possible (log warning may be?). As for the user of the library, it is logical and reasonable expectation, but this is just my opinion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559534171
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
Yeah, in this particular case for ArrayList it could be certainly done, I run into it by accindent ;-). I am wondering if you were able to see the other failures, `ReadPrimitiveTest` & `CircularExceptionTest`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta edited a comment on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761658880
Hey @rmannibucau , thanks for taking a look:
> 1. Optional usage can be dropped
Could drop it but replace with `null` checks?
> 2. Accessibility can be tested
Build with JDK16 or run test with JDK16, I could help with that on Jenkins fe.
> 3. Accessibility can still be enforced (worse case adding a build time generation) - can help on that in ~7dayd
This part is unclear to me in a sense, those are internal JDK classes, how would build time generation help?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761665453
Overall null check is not needed, entry is only added if accessible.
Generator using TheClass$Accessor naming can enable to make public private fields without reflection. Can be a workaround but we need to define when accessibility is lost and needed. Regressions there are not that acceptable for the spec and future :s.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766146434
@rmannibucau for my issue with `ArrayList` - definitely enough, do you want to address `InaccessibleObjectException` in general in scope of this PR? I have collected enough details regarding the issue but have not updated the change yet.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761689210
@rmannibucau comments addressed, the only regression I have seen so far across the test suite is this guy:
```
@Test
public void dontStackOverFlowUsingFieldAccess() {
final Throwable oopsImVicous = new Exception("circular");
oopsImVicous.getStackTrace(); // fill it
oopsImVicous.initCause(new IllegalArgumentException(oopsImVicous));
final String serialized = new MapperBuilder().setAccessModeName("field").build().writeObjectAsString(oopsImVicous);
assertTrue(serialized.contains("\"detailMessage\":\"circular\""));
assertTrue(serialized.contains("\"stackTrace\":[{"));
}
```
Essentially, with `field` access mode, none of the `Exception` fields are accessible and `serialized ` is effectively empty.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766732201
@reta added some basic exception support: https://github.com/apache/johnzon/commit/4f53103f64c9f7c0ec645c1b0d04c1d34f9a8dcf .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761658880
Hey @rmannibucau , thanks for taking a look:
> 1. Optional usage can be dropped
Could drop it but replace with `null` checks?
> 2. Accessibility can be tested
Build with JDK16 or run test with JDK16, I could help with that on Jenkins fe.
> 3. Accessibility can still be enforced (worse case adding a build time generation) - can help on that in ~7dayd
This part is unclear to me in a sense, those are internal JDK classes, how would build time generation help?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau edited a comment on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau edited a comment on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761825387
@reta tested with master (plain master, not patched) and it works well on java 16 so guess you went into a particular case where we try to do reflection on jdk classes (java.util.ArrayList.size it seems) which is something we can avoid with a "if known() then do something clever" earlier in introspection process so I think we don't need to do anything about setAccessible but rather we should fix the cause of this arraylist introspection (maybe hashmap too). Any way to write a reproducer test?
edit: can be interesting to have the frame before `.Mapper.writeObject` since for a list it shouldnt enter in this method but writeCollection or friends.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta closed pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta closed pull request #73:
URL: https://github.com/apache/johnzon/pull/73
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta edited a comment on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761689210
@rmannibucau comments addressed, the only regression I have seen so far across the test suite is this guy:
```
@Test
public void dontStackOverFlowUsingFieldAccess() {
final Throwable oopsImVicous = new Exception("circular");
oopsImVicous.getStackTrace(); // fill it
oopsImVicous.initCause(new IllegalArgumentException(oopsImVicous));
final String serialized = new MapperBuilder().setAccessModeName("field").build().writeObjectAsString(oopsImVicous);
assertTrue(serialized.contains("\"detailMessage\":\"circular\""));
assertTrue(serialized.contains("\"stackTrace\":[{"));
}
```
Essentially, with `field` access mode, none of the `Exception` fields are accessible and `serialized ` is effectively empty. Personally, I don't think this is a concern since with `method` access level things work fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761884192
@rmannibucau Yes, I do have a test but I am very surprised with your outcomes, I saw a number (but not many) of test failures. Just to double check, you used latest JDK-16 EA build, right? (the JEP in question was integrated ~ month ago). Could you also try JDK17-ea? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559389372
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
Interesting, just read on the phone but our list detection is something along "is parameterized and raw type is a collection or is an array" so passing List.class was moving it to a fallback handling and lead to the unexpected stack you get.
Guess it is what we must fix and probably for maps too.
Hope it makes sense
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559664321
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
Let's keep it open until we review the final solution and agree on it. Please ping me if you dont get news in 10 days ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761825387
@reta tested with master (plain master, not patched) and it works well on java 16 so guess you went into a particular case where we try to do reflection on jdk classes (java.util.ArrayList.size it seems) which is something we can avoid with a "if known() then do something clever" earlier in introspection process so I think we don't need to do anything about setAccessible but rather we should fix the cause of this arraylist introspection (maybe hashmap too). Any way to write a reproducer test?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta edited a comment on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761884192
@rmannibucau Yes, I do have a test but I am very surprised with your outcomes, I saw a number (but not many) of test failures. Just to double check, you used latest JDK-16 EA build, right? (the JEP in question was integrated ~ month ago). Could you also try JDK17-ea? Thanks.
Edit: Just a number of different failures in `ReadPrimitiveTest` & `CircularExceptionTest` fe:
```
java.lang.reflect.InaccessibleObjectException: Unable to make field private final char java.lang.Character.value accessible: module java.base does not "opens java.lang" to unnamed module @33833882
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldDecoratedType.<init>(FieldAccessMode.java:105)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldReader.<init>(FieldAccessMode.java:169)
at org.apache.johnzon.mapper.access.FieldAccessMode.doFindReaders(FieldAccessMode.java:49)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.mapper.access.FieldAndMethodAccessMode.doFindReaders(FieldAndMethodAccessMode.java:68)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.mapper.Mappings.createClassMapping(Mappings.java:475)
...
at org.apache.johnzon.mapper.Mapper.readObject(Mapper.java:295)
at org.apache.johnzon.mapper.ReadPrimitiveTest.testCharacter(ReadPrimitiveTest.java:48)
```
```
java.lang.reflect.InaccessibleObjectException: Unable to make field private java.lang.Throwable java.lang.Throwable.cause accessible: module java.base does not "opens java.lang" to unnamed module @33833882
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldDecoratedType.<init>(FieldAccessMode.java:105)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldReader.<init>(FieldAccessMode.java:169)
at org.apache.johnzon.mapper.access.FieldAccessMode.doFindReaders(FieldAccessMode.java:49)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.mapper.Mappings.createClassMapping(Mappings.java:475)
at org.apache.johnzon.mapper.Mappings.doFindOrCreateClassMapping(Mappings.java:437)
...
at org.apache.johnzon.mapper.Mapper.writeObjectAsString(Mapper.java:252)
at org.apache.johnzon.mapper.CircularExceptionTest.dontStackOverFlow(CircularExceptionTest.java:31)
```
I will shortly extract the test for `ArrayList` as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766789071
Closing as not needed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766141778
@reta can you review https://issues.apache.org/jira/browse/JOHNZON-331 and the related commit (https://github.com/apache/johnzon/commit/15a4cc69344b3a0dd7ee80326cc064fa0972a779). Does it solve your issue or do we need "more"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559614298
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
@rmannibucau no, it is not urgent, please let me know if I could help in scope of this PR (or I can just close it).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766170860
@rmannibucau thanks, let me just keep it open for a few days if someone has opinion on the matter, otherwise let us just close it as not needed, wdyt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761759278
Hi @reta, had a quick look. Seems it is only an issue if in a not yet opened module so we should be able yo skip fields in this case no - and not have to catch it as a fallback. Wdyt? Can help on code side in 7days.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559541629
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
Didnt run it but looks like it is the same issue of "too late type handling" so guess fix is to move it earlier trying to limit runtime impact.
If not urgent can work on it next week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761752091
I care more of a regression out of the test suite, guess running tck is an option. Test suite will get some priviledged access. If no regression, not sure the change is useful too so we should refine it i think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761725009
The Travis build failed but issue seems to be unrelated:
```
[ERROR] Failed to execute goal on project apache-johnzon: Could not resolve dependencies for project org.apache.johnzon:apache-johnzon:jar:1.2.11-SNAPSHOT: The following artifacts could not be resolved: org.apache.johnzon:johnzon-core:jar:javadoc:1.2.11-SNAPSHOT, org.apache.johnzon:johnzon-mapper:jar:javadoc:1.2.11-SNAPSHOT, org.apache.johnzon:johnzon-jaxrs:jar:javadoc:1.2.11-SNAPSHOT, org.apache.johnzon:johnzon-websocket:jar:javadoc:1.2.11-SNAPSHOT, org.apache.johnzon:johnzon-maven-plugin:jar:javadoc:1.2.11-SNAPSHOT, org.apache.johnzon:johnzon-jsonb:jar:javadoc:1.2.11-SNAPSHOT: Could not find artifact org.apache.johnzon:johnzon-core:jar:javadoc:1.2.11-SNAPSHOT in sonatype-snapshots (https://oss.sonatype.org/content/repositories/snapshots/) -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/DependencyResolutionException
[ERROR]
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR] mvn <args> -rf :apache-johnzon
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r559274808
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
@rmannibucau Here is the test, consistently fails (for me) on the master (unpatched) on JDK16-ea+28 and above:
```
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.johnzon.jsonb.JsonbWriteTest
[ERROR] Tests run: 12, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.27 s <<< FAILURE! - in org.apache.johnzon.jsonb.JsonbWriteTest
[ERROR] listOfSimple(org.apache.johnzon.jsonb.JsonbWriteTest) Time elapsed: 0.016 s <<< ERROR!
java.lang.reflect.InaccessibleObjectException: Unable to make field private int java.util.ArrayList.size accessible: module java.base does not "opens java.util" to unnamed module @6b71769e
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldDecoratedType.<init>(FieldAccessMode.java:105)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldReader.<init>(FieldAccessMode.java:169)
at org.apache.johnzon.mapper.access.FieldAccessMode.doFindReaders(FieldAccessMode.java:49)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.mapper.access.FieldAndMethodAccessMode.doFindReaders(FieldAndMethodAccessMode.java:68)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.jsonb.JsonbAccessMode.findReaders(JsonbAccessMode.java:468)
at org.apache.johnzon.mapper.Mappings.createClassMapping(Mappings.java:475)
at org.apache.johnzon.mapper.Mappings.doFindOrCreateClassMapping(Mappings.java:437)
at org.apache.johnzon.mapper.Mappings.findOrCreateClassMapping(Mappings.java:411)
at org.apache.johnzon.mapper.Mapper.isDeduplicateObjects(Mapper.java:215)
at org.apache.johnzon.mapper.Mapper.writeObjectWithGenerator(Mapper.java:209)
at org.apache.johnzon.mapper.Mapper.writeObject(Mapper.java:203)
at org.apache.johnzon.mapper.Mapper.writeObjectAsString(Mapper.java:252)
at org.apache.johnzon.jsonb.JohnzonJsonb.toJson(JohnzonJsonb.java:342)
at org.apache.johnzon.jsonb.JsonbWriteTest.listOfSimple(JsonbWriteTest.java:102)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761815648
>
>
> I care more of a regression out of the test suite, guess running tck is an option. Test suite will get some priviledged access. If no regression, not sure the change is useful too so we should refine it i think.
Running TCK would be good, the regressions should be unlikely (🤞 ), from the access checks I have run into, `Johnzon` just introspects more than it needs (probably) in fact.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766159045
@reta question is: can it still happen in a valid setup (mapping a not opened/imported model is assumed wrong i guess). I didnt have more issues on java 16 so for me this exception is gone but if you see other cases we must fix them. Maybe exception case? Didnt check this last one but fix is likely to map it directly and not by reflection. Wdyt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r560154579
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
@rmannibucau This very correct, I will continue digging in so we could be proactive in introspection (we are indeed reactive now), will be back on that :) Thanks for the feedback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766732201
@reta added some basic exception support: https://github.com/apache/johnzon/commit/4f53103f64c9f7c0ec645c1b0d04c1d34f9a8dcf .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766789071
Closing as not needed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta edited a comment on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-761658880
Hey @rmannibucau , thanks for taking a look:
> 1. Optional usage can be dropped
Could drop it but replace with `null` checks?
> 2. Accessibility can be tested
Build with JDK16 or run test with JDK16, I could help with that on Jenkins fe.
> 3. Accessibility can still be enforced (worse case adding a build time generation) - can help on that in ~7dayd
This part is unclear to me in a sense, those are internal JDK classes, how would build time generation help?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] reta closed pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
reta closed pull request #73:
URL: https://github.com/apache/johnzon/pull/73
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on a change in pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #73:
URL: https://github.com/apache/johnzon/pull/73#discussion_r560041836
##########
File path: johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/JsonbWriteTest.java
##########
@@ -92,6 +94,14 @@ public void list() {
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map));
}
+ @Test
+ public void listOfSimple() {
Review comment:
Hi @reta,
I like this version more but it still fixes the consequence instead of the source of the issue which is an unexpected branch in the mapper (class for collection/primitive/map/throwable/jdk-class) so even if this flavor can be kept I still think we should fix first the cause then maybe review this flavor.
Wdyt?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #73: Fix InaccessibleObjectException on JDK16+
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #73:
URL: https://github.com/apache/johnzon/pull/73#issuecomment-766165691
Well my reasoning is:
1. Jpms is not adopted and will not probably so maybe not a big deal
2. If not opened it is likely intended in jpms land so probably better to respect it in the spec (was not discussed)
So overall your impl could be a toggle but *my* feeling is that it shouldnt be the default if we want to support jpms correctly - but once again jpms can be dead today so maybe not a big deal.
Side note: no logging is likely desired so failling or swallowing probably?
Anyone else has an opinion on it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org