You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by "jungm (via GitHub)" <gi...@apache.org> on 2023/09/25 13:50:42 UTC
[GitHub] [johnzon] jungm opened a new pull request, #110: Implement JSON-P 2.1
jungm opened a new pull request, #110:
URL: https://github.com/apache/johnzon/pull/110
Fixes the currently failing JSON-P 2.1 TCK tests (and implements what was missing for JSON-P 2.1 along the way).
Right now only pluggability and SigTest TCK tests are failing, but those are related to the TCK not being set up properly as far as I can tell:
```
[ERROR] Failures:
[ERROR] ClientTests.jsonProviderTest1:103 jsonProviderTest1 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest11:343 jsonProviderTest11 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest12:365 jsonProviderTest12 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest13:388 jsonProviderTest13 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest14:410 jsonProviderTest14 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest15:433 jsonProviderTest15 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest16:455 jsonProviderTest16 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest17:477 jsonProviderTest17 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest18:500 jsonProviderTest18 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest2:132 jsonProviderTest2 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest3:162 jsonProviderTest3 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest4:181 jsonProviderTest4 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest5:204 jsonProviderTest5 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest6:227 jsonProviderTest5 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest7:250 jsonProviderTest7 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest8:273 jsonProviderTest8 Failed ==> expected: <true> but was: <false>
[ERROR] ClientTests.jsonProviderTest9:296 jsonProviderTest9 Failed ==> expected: <true> but was: <false>
[ERROR] Errors:
[ERROR] JSONPSigTest.signatureTest:281 Fault JSONPSigTest.signatureTest() failed!, diffs found
[INFO]
[ERROR] Tests run: 197, Failures: 17, Errors: 1, Skipped: 0
```
Draft PR for now because of two backwards-compatibility questions I'd like to discuss before merging:
- keep `JohnzonJsonParser#current()`? It kind of is in the specs as `JsonParser#currentEvent()` now
- continue to support the `johnzon.rejectDuplicateKeys` and `org.glassfish.json.rejectDuplicateKeys` properties for the duplicate keys mode? I'd at least like to drop the glassfish one as I can't find it anymore anywhere in jsonp 2.1 specs
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1753576778
Excellent work.
Thank you so much for the great contribution.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii merged PR #110:
URL: https://github.com/apache/johnzon/pull/110
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jungm commented on pull request #110: Implement JSON-P 2.1
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1734110352
@rmannibucau current has a tiny functional difference (Johnzon one advances to the first event while jsonp doesn't).
But this behaviour also isn't even really documented, see:
```java
/**
* @return the _current_ Event. That's the one returned by the previous call to {@link #next()}
* but without propagating the Event pointer to the next entry.
*/
default Event current() {
throw new UnsupportedOperationException("getting the current JsonParser Event is not supported");
}
```
I'm fine with keeping it but I'd at least update the javadoc that it _does_ move the event pointer under certain circumstances
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1754669255
I propose we merge this one and we introduce a flag afterwards to make the #close() behavior back to normal and introduce a flag for the TCK compliance
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1752952333
Hi
checked quickly the diff and we must keep DEFAULT for the strategy for backward compatibility (it is used) and from() method must also stay backward compatible so I wouldn't rename anything.
We can read a system property to override the default behavior in factories if it helps and for tck setup.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1753045317
@rmannibucau reverted the renaming now and also reverted most of the from() method but added a few more alias mappings to stay json-p compliant
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351606061
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
Hmm, how do we guarantee writer is closed when ex != null? didnt find the call and I don't see us providing a leaking impl cause we had an exception - we can request the spec to fix this bug which was not there before if it is written down if not we just challenge the tck
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351695668
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
We guarantee the writer is _not_ closed when an exception is thrown, looks like this has been specified for json-p 2.1 [here](https://github.com/jakartaee/jsonp-api/issues/156)
Also see [javadocs](https://jakarta.ee/specifications/jsonp/2.1/apidocs/jakarta.json/jakarta/json/stream/jsongenerator#close()):
> The underlying stream is closed only if complete JSON object is written. In case of incomplete object JsonGenerationException is thrown and underlying stream is not closed.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351741172
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
Ok, then let's add a toggle disabled by default to support that in the tck module, it is not responsible to leak by default IMHO
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351538298
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
That one is an intended change to pass [jsonGeneratorStreamNotClosedTest](https://github.com/jakartaee/jsonp-api/blob/2.1.1-tck/tck/tck-tests/src/main/java/ee/jakarta/tck/jsonp/api/jsongeneratortests/ClientTests.java#L2094)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351698254
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
Here is the explaination https://github.com/jakartaee/jsonp-api/issues/156
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1350385691
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
typo?
##########
johnzon-core/src/main/java/org/apache/johnzon/core/RejectDuplicateKeysMode.java:
##########
@@ -44,14 +48,16 @@ public static RejectDuplicateKeysMode from(final Map<String, ?> config) {
if (config == null) {
return DEFAULT;
}
+
return CONFIG_KEYS.stream()
.map(config::get)
.filter(Objects::nonNull)
.findFirst()
.map(String::valueOf)
- .map(it -> "false".equalsIgnoreCase(it) ? "DEFAULT" : it) // alias to avoid to add an enum value for nothing
+ .map(it -> "false".equalsIgnoreCase(it) || "LAST".equalsIgnoreCase(it) ? "DEFAULT" : it) // aliases to avoid to add an enum value for nothing
+ .map(it -> "NONE".equalsIgnoreCase(it) ? "true" : it)
.map(it -> valueOf(it.toUpperCase(Locale.ROOT).trim()))
- .orElse(DEFAULT);
+ .orElse(TRUE);
Review Comment:
why not default (backward compat)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351694347
##########
johnzon-core/src/main/java/org/apache/johnzon/core/JsonGeneratorImpl.java:
##########
@@ -449,8 +449,8 @@ public void close() {
try {
if (ex == null) {
flushBuffer();
+ writer.close();
Review Comment:
We can't challenge the TCK. The javadoc clearly says it https://jakarta.ee/specifications/jsonp/2.1/apidocs/jakarta.json/jakarta/json/stream/jsongenerator#close()
> The underlying stream is closed only if complete JSON object is written. In case of incomplete object JsonGenerationException is thrown and underlying stream is not closed.
We can create an issue to fix the specification and therefor fix the TCK accordingly. But as per the current specification version, the challenge would be invalid. I have no idea what was the reason.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on code in PR #110:
URL: https://github.com/apache/johnzon/pull/110#discussion_r1351539208
##########
johnzon-core/src/main/java/org/apache/johnzon/core/RejectDuplicateKeysMode.java:
##########
@@ -44,14 +48,16 @@ public static RejectDuplicateKeysMode from(final Map<String, ?> config) {
if (config == null) {
return DEFAULT;
}
+
return CONFIG_KEYS.stream()
.map(config::get)
.filter(Objects::nonNull)
.findFirst()
.map(String::valueOf)
- .map(it -> "false".equalsIgnoreCase(it) ? "DEFAULT" : it) // alias to avoid to add an enum value for nothing
+ .map(it -> "false".equalsIgnoreCase(it) || "LAST".equalsIgnoreCase(it) ? "DEFAULT" : it) // aliases to avoid to add an enum value for nothing
+ .map(it -> "NONE".equalsIgnoreCase(it) ? "true" : it)
.map(it -> valueOf(it.toUpperCase(Locale.ROOT).trim()))
- .orElse(DEFAULT);
+ .orElse(TRUE);
Review Comment:
Was an oversight when renaming everything back, is fixed now
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement JSON-P 2.1 [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1752927981
I rebased on latest master again now to make running TCKs easier. This branch fully passes all JSON-P 2.1 TCK tests with #111 being merged now.
Feel free to go ahead and review my changes
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #110: Implement JSON-P 2.1
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #110:
URL: https://github.com/apache/johnzon/pull/110#issuecomment-1733844541
For current no pref if iso (functionally and perf), for the props please keep both for compat.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org