search

leadpony / jsonp-test-suite

Test Suite for implementations of Jakarta JSON Processing API (JSON-P)
Apache License 2.0
3 stars 2 forks source link

Hex escapes should allow lower- or upper-case letters A-F #1

Closed toddobryan closed 4 years ago

toddobryan commented 4 years ago

I'm checking an implementation of JsonP I'm writing based on Jackson and got these two errors:

Expected :"hello\u000bworld"
Actual   :"hello\u000Bworld"

Expected :"hello\u001bworld"
Actual   :"hello\u001Bworld"

Both of these should be correct, I think.

I can't change the escaping because I'm relying on the underlying Jackson code.

leadpony commented 4 years ago

Hello @toddobryan Would you please tell me which test class are you trying?

leadpony commented 4 years ago

Now I identified the error location.

toddobryan commented 4 years ago

Sorry about that. I wasn't sure the right way to fix it or I would have given it a try.

leadpony commented 4 years ago

It's OK. I will fix the problem in a few days. Thank you for trying this test suite.

leadpony commented 4 years ago

HI @toddobryan I fixed the problem. Could you please test your implementation with the current master snapshot?

<dependency>
    <groupId>org.leadpony</groupId>
    <artifactId>jsonp-test-suite</artifactId>
    <version>master-SNAPSHOT</version>
</dependency>
toddobryan commented 4 years ago

Will do. Thanks again! Also, this suite caught a mistake in Jackson Core for which I've submitted a pull request: https://github.com/FasterXML/jackson-core/pull/557

I leaned heavily on your test structure, but had to change it a bit because of the difference in the way Jackson specifies locations. I may have borrowed enough of your code that they'd like you to provide a contributor license agreement, if you don't mind. If you do mind, I'll rework everything so that it doesn't depend on your code.

toddobryan commented 4 years ago

Works like a charm now! Thank you!

leadpony commented 4 years ago

Hello @toddobryan Thank you for your feedback. I am glad hear it works properly for you. I do not mind to accept the license agreement, but I think it is better for you that you rework or minimize the test cases copied. I am the author of a JSON parser, which is one of JSON-P implementation, therefore I may be a small competitor for Jackson's team.

toddobryan commented 4 years ago

Since the tests aren't distributed with the library, they were okay with them. Here's the commit...if there's anything in there that you would like me to remove, let me know.

https://github.com/FasterXML/jackson-core/pull/557/files

toddobryan commented 4 years ago

The reason this even exists is that I was working on a JSON-P implementation based on Jackson, so your tests caught problems in Jackson that I had converted to JSON-P.

I've reached the point where a lot of the Jackson conversion is done, but there's still a lot of JSON-P to implement--all of the Patch, Pointer, etc., stuff that really seems like, once you've converted from another representation into the javax.json structures, you could just use a generic implementation of those.

I was thinking of using Joy for that, but wanted to make sure that you don't object. The idea would be that I'd implement JsonValue and its subclasses based on Jackson and then defer to the Joy implementations of things like JsonPointer, JsonPatch, etc. But that's using an awful lot of your code to let people use another parser library, so I wanted to ask you first.

leadpony commented 4 years ago

I was thinking of using Joy for that, but wanted to make sure that you don't object. The idea would be that I'd implement JsonValue and its subclasses based on Jackson and then defer to the Joy implementations of things like JsonPointer, JsonPatch, etc. But that's using an awful lot of your code to let people use another parser library, so I wanted to ask you first.

No I don't object to it. Joy is licensed under Apache License 2.0, so you can freely use it if and only if the license conditions are fully met. For example, you copied the files in this repository but your repository seems to lack NOTICE file, which may be considered as a license violation. Please see 4. Redistribution. d in the Apache License 2.0. If you need further discussion in regards to Joy, please create a new issue at Joy. Thank you.

toddobryan commented 4 years ago

Major apologies. I should not have posted those files. I just copied them because I was too lazy to figure out how to run the tests by depending on the jar and don't intend them to be part of the final repo. I'm traveling this weekend, but as soon as I get home, I'll create a new repo without the file, and will add a README with a big thank you for the test suite. Also, if you'd prefer I not use issue comments for communication, my email is toddobryan@gmail.com and I'd be happy to use email instead.

leadpony commented 4 years ago

@toddobryan Do not worry about it. It is a trivial mistake anyone does.

leadpony commented 4 years ago

I close this issue because the bug is fixed in the latest release 1.4.0. Please see also the changelog. Thank you.