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 2020/12/02 10:59:27 UTC
[GitHub] [johnzon] jeanouii opened a new pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
jeanouii opened a new pull request #70:
URL: https://github.com/apache/johnzon/pull/70
Not sure if this is what the discussion was suggesting
Signed-off-by: Jean-Louis Monteiro <jl...@tomitribe.com>
----------------------------------------------------------------
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 #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #70:
URL: https://github.com/apache/johnzon/pull/70#discussion_r534089716
##########
File path: johnzon-core/src/main/java/org/apache/johnzon/core/JsonProviderImpl.java
##########
@@ -53,6 +53,12 @@
import javax.json.stream.JsonParserFactory;
public class JsonProviderImpl extends JsonProvider implements Serializable {
+
+ /**
+ * This makes Johnzon to be compliant with spec, essentially regarding the JSON Pointer /-
+ */
+ private final boolean isStrictCompliance = Boolean.getBoolean("org.apache.johnzon.STRICT_JSONP_COMPLIANCE");
Review comment:
a few notes on this one (hopefully from he most trivial to the design ones):
0. naming convention is aligned on tomcat but not johnzon (strict-jsonp-complance)
1. in jsonpointer constructor it should likely be converted to an int directly to avoid the if (minor)
2. makes me wonder about our default, we discussed on the list of a potentially breaking change in 1.3 but this flag as such will likely never be set and stay forever which does not sound very good to me
3. I know we already use a lot system properties but it is mainly for tuning/perf, here it is about the application behavior which means it can be set programmatically in multi app containers (I'm more thinking about OSGi than EE here) which can make it random so I'm quite mitigated
4. I dont know if you had a look but in openwebbeans we use cdi-se standalone SPI and we added a spi behind it to enable to plug another impl (owb, meecrowave, tomee, ...). We can probably do the same, ie add a JohnzonJsonPointerFactory SPI we load with the provider (plain serviceloader, and we take the one with the higheest ordinal/priority to avoid to fail if there are 2 in hierarchical classloaders). Then we can add a johnzon-jsonp-strictpointer module (if you find a better name ;)). This solves it without the need of the flag. I'm happy to help on this one if needed but think it can enable this flag without the pitfall of a global toggle like this one (in OSGi it would be linked or not, in EE it is present or not etc).
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] jeanouii commented on pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
jeanouii commented on pull request #70:
URL: https://github.com/apache/johnzon/pull/70#issuecomment-737347364
I merged your PR into mine so everything should now be consistent and fixed
----------------------------------------------------------------
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] jeanouii commented on a change in pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
jeanouii commented on a change in pull request #70:
URL: https://github.com/apache/johnzon/pull/70#discussion_r534096922
##########
File path: johnzon-core/src/main/java/org/apache/johnzon/core/JsonProviderImpl.java
##########
@@ -53,6 +53,12 @@
import javax.json.stream.JsonParserFactory;
public class JsonProviderImpl extends JsonProvider implements Serializable {
+
+ /**
+ * This makes Johnzon to be compliant with spec, essentially regarding the JSON Pointer /-
+ */
+ private final boolean isStrictCompliance = Boolean.getBoolean("org.apache.johnzon.STRICT_JSONP_COMPLIANCE");
Review comment:
> a few notes on this one (hopefully from he most trivial to the design ones):
>
> 1. naming convention is aligned on tomcat but not johnzon (strict-jsonp-complance)
Done
> 2. in jsonpointer constructor it should likely be converted to an int directly to avoid the if (minor)
Well in JsonPointer it's an int, but it might be something else in the future and something else for other objects. So I'm passing the flag and let the objects to handle it.
> 3. makes me wonder about our default, we discussed on the list of a potentially breaking change in 1.3 but this flag as such will likely never be set and stay forever which does not sound very good to me
We are in 1.2.9-SNAPSHOT so default must be backward compatible as far as I understood from the discussion.
But in essence, you are right, aside from having the stamp, no one will really care about it.
> 4. I know we already use a lot system properties but it is mainly for tuning/perf, here it is about the application behavior which means it can be set programmatically in multi app containers (I'm more thinking about OSGi than EE here) which can make it random so I'm quite mitigated
> 5. I dont know if you had a look but in openwebbeans we use cdi-se standalone SPI and we added a spi behind it to enable to plug another impl (owb, meecrowave, tomee, ...). We can probably do the same, ie add a JohnzonJsonPointerFactory SPI we load with the provider (plain serviceloader, and we take the one with the higheest ordinal/priority to avoid to fail if there are 2 in hierarchical classloaders). Then we can add a johnzon-jsonp-strictpointer module (if you find a better name ;)). This solves it without the need of the flag. I'm happy to help on this one if needed but think it can enable this flag without the pitfall of a global toggle like this one (in OSGi it would be linked or not, in EE it is present or not etc).
Don't mind but not sure I can address this now though.
And as mentioned in 3/ no one will use this property unless you want to be certified. So can be seen as tuning/perf.
>
> 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] jgallimore commented on a change in pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
jgallimore commented on a change in pull request #70:
URL: https://github.com/apache/johnzon/pull/70#discussion_r534094316
##########
File path: johnzon-core/src/main/java/org/apache/johnzon/core/JsonProviderImpl.java
##########
@@ -53,6 +53,12 @@
import javax.json.stream.JsonParserFactory;
public class JsonProviderImpl extends JsonProvider implements Serializable {
+
+ /**
+ * This makes Johnzon to be compliant with spec, essentially regarding the JSON Pointer /-
+ */
+ private final boolean isStrictCompliance = Boolean.getBoolean("org.apache.johnzon.STRICT_JSONP_COMPLIANCE");
Review comment:
4. I did wonder the same, but then we potentially have a scenario where the app server needs the setting to pass that TCK, and the setting isn't available in when using Johnzon standalone - you then can't get the same behaviour in both. Unit-testing is a use case where I could see this being useful, for example.
While I'm sure we could solve this in the app server, I think my own preference would be to enable the behaviour in Johnzon itself.
----------------------------------------------------------------
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] jgallimore commented on a change in pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
jgallimore commented on a change in pull request #70:
URL: https://github.com/apache/johnzon/pull/70#discussion_r534091752
##########
File path: johnzon-core/src/main/java/org/apache/johnzon/core/JsonProviderImpl.java
##########
@@ -53,6 +53,12 @@
import javax.json.stream.JsonParserFactory;
public class JsonProviderImpl extends JsonProvider implements Serializable {
+
+ /**
+ * This makes Johnzon to be compliant with spec, essentially regarding the JSON Pointer /-
Review comment:
Given that the TCK is open, I'd suggest referencing the specific test case here. Not so much for a javadoc point of view, but more to track the history. The JSR (as far as I can see) doesn't have any spec text at all, so a developer going looking for it, will at best arrive at the RFC.
The setting is necessary for Johnzon to be compliant with JSR-374 / Jakarta JSON Processing (https://github.com/eclipse-ee4j/jsonp/), which specifies the "/-" reference points to the index _after_ the last element in the array: https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/com/sun/ts/tests/jsonp...<whichever 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] rmannibucau commented on pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #70:
URL: https://github.com/apache/johnzon/pull/70#issuecomment-737190768
Hopefully making it clearer here is the toggle proposal I mentionned: https://github.com/apache/johnzon/pull/71
----------------------------------------------------------------
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] jeanouii merged pull request #70: JOHNZON-325 see if we can get something that makes everyone happy
Posted by GitBox <gi...@apache.org>.
jeanouii merged pull request #70:
URL: https://github.com/apache/johnzon/pull/70
----------------------------------------------------------------
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