[GitHub] [johnzon] jeanouii commented on a change in pull request #70: JOHNZON-325 see if we can get something that makes everyone happy

[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