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/07/26 10:20:52 UTC
[GitHub] [johnzon] markuung opened a new pull request #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
markuung opened a new pull request #67:
URL: https://github.com/apache/johnzon/pull/67
This resolves JOHNZON-319
----------------------------------------------------------------
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 #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r462057515
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
+ config.getProperty(JsonParserFactoryImpl.BUFFER_LENGTH).map(Integer.class::cast).ifPresent(builder::setBufferSize);
+ config.getProperty(JsonParserFactoryImpl.MAX_STRING_LENGTH).map(Integer.class::cast).ifPresent(builder::setMaxSize);
+ config.getProperty(JsonParserFactoryImpl.SUPPORTS_COMMENTS).map(Boolean.class::cast).ifPresent(builder::setSupportsComments);
+ config.getProperty(JsonParserFactoryImpl.ENCODING).map(String::valueOf).ifPresent(builder::setEncoding);
Review comment:
Hmm, any PR must not use as jsonb properties some jsonp properties by design or it should use a filter like + mapping algorithm (prefix or so) to create a kind of passthrough mode for properties. Aliases are current way to code it, subnamespace would work too. Jsonb module must also not include johnzon-core imports. in these regards this PR is not mergeable since it creates a naming inconsistency and breaks module design, this is why I mentionned it.
Thinking out loud i wouldnt map all properties for jsonp but a single "properties" (map) one and wire it in all jsonb config consumers (jaxrs providers for ex).
----------------------------------------------------------------
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 #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r460542429
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
+ config.getProperty(JsonParserFactoryImpl.BUFFER_LENGTH).map(Integer.class::cast).ifPresent(builder::setBufferSize);
+ config.getProperty(JsonParserFactoryImpl.MAX_STRING_LENGTH).map(Integer.class::cast).ifPresent(builder::setMaxSize);
+ config.getProperty(JsonParserFactoryImpl.SUPPORTS_COMMENTS).map(Boolean.class::cast).ifPresent(builder::setSupportsComments);
+ config.getProperty(JsonParserFactoryImpl.ENCODING).map(String::valueOf).ifPresent(builder::setEncoding);
Review comment:
Must use jsonb property, not jsonp one
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
Review comment:
Must use johnzon.name pattern IMHO for consistency and not leak jsonp impl properties since we support other jsonp impl and already used shortnames for other feature.
----------------------------------------------------------------
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] jungm commented on a change in pull request #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
jungm commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r462051933
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
+ config.getProperty(JsonParserFactoryImpl.BUFFER_LENGTH).map(Integer.class::cast).ifPresent(builder::setBufferSize);
+ config.getProperty(JsonParserFactoryImpl.MAX_STRING_LENGTH).map(Integer.class::cast).ifPresent(builder::setMaxSize);
+ config.getProperty(JsonParserFactoryImpl.SUPPORTS_COMMENTS).map(Boolean.class::cast).ifPresent(builder::setSupportsComments);
+ config.getProperty(JsonParserFactoryImpl.ENCODING).map(String::valueOf).ifPresent(builder::setEncoding);
Review comment:
I just don't think this PR should also contain the aliasing of the jsonp properties to jsonb-style properties. But if you think otherwise I'll include 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] markuung commented on a change in pull request #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
markuung commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r460554542
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
+ config.getProperty(JsonParserFactoryImpl.BUFFER_LENGTH).map(Integer.class::cast).ifPresent(builder::setBufferSize);
+ config.getProperty(JsonParserFactoryImpl.MAX_STRING_LENGTH).map(Integer.class::cast).ifPresent(builder::setMaxSize);
+ config.getProperty(JsonParserFactoryImpl.SUPPORTS_COMMENTS).map(Boolean.class::cast).ifPresent(builder::setSupportsComments);
+ config.getProperty(JsonParserFactoryImpl.ENCODING).map(String::valueOf).ifPresent(builder::setEncoding);
Review comment:
Completely oversaw that I used the jsonp property here for the encoding, thanks!
You however marked 4 lines with this comment, all the other properties look very specific to the json-p impl. Am I missing something here?
----------------------------------------------------------------
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] markuung commented on a change in pull request #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
markuung commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r460554609
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
Review comment:
JohnzonBuilder#generatorConfig() and JohnzonBuilder#readerConfig() also use org.apache.johnzon.* like properties, also exposing json-p impl specific properties, so I reused those. Should those maybe be changed too?
----------------------------------------------------------------
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 #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r460560295
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
+ config.getProperty(JsonParserFactoryImpl.BUFFER_LENGTH).map(Integer.class::cast).ifPresent(builder::setBufferSize);
+ config.getProperty(JsonParserFactoryImpl.MAX_STRING_LENGTH).map(Integer.class::cast).ifPresent(builder::setMaxSize);
+ config.getProperty(JsonParserFactoryImpl.SUPPORTS_COMMENTS).map(Boolean.class::cast).ifPresent(builder::setSupportsComments);
+ config.getProperty(JsonParserFactoryImpl.ENCODING).map(String::valueOf).ifPresent(builder::setEncoding);
Review comment:
Johnzon properties are all renamed with shorter aliases in jsonb binding ("johnzon.x"). Jsonp properties are often way too long so think it is worth aligning it on this convention.
----------------------------------------------------------------
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 #67: [JOHNZON-319] Map all properties supported by MapperBuilder in JohnzonBuilder
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #67:
URL: https://github.com/apache/johnzon/pull/67#discussion_r462160092
##########
File path: johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java
##########
@@ -131,9 +131,12 @@ public Jsonb build() {
return it;
}).orElse(false);
- if (config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).orElse(false)) {
- builder.setPretty(true);
- }
+ config.getProperty(JsonbConfig.FORMATTING).map(Boolean.class::cast).ifPresent(builder::setPretty);
+ config.getProperty(AbstractJsonFactory.BUFFER_STRATEGY).map(String::valueOf).ifPresent(builder::setBufferStrategy);
Review comment:
Yep, it shouldn't have been.
Was ok-ish in standalone/ee because it was not in the main case where jsonp impl was loaded automatically so not that visible in terms of classloading and runtime but this breaks the module design (and OSGi metadata).
It is fine to copy these strings in the builder (inline if triggered cause jsonp != null for backward compat) and we should just use a Map<String, ?> for the main case (single property, for instance "johnzon.jsonp.readerfactory.properties" and "johnzon.jsonp.generator.properties" - <jsonbimpl>.<config area>.<component>.<property> pattern?).
Will enable to handle any jsonp impl configuration and future properties addition without anything 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