Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
toddobryan opened this issue Sep 3, 2019 · 14 comments
Closed

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

toddobryan opened this issue Sep 3, 2019 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@toddobryan
Copy link

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
Copy link
Owner

leadpony commented Sep 3, 2019

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

@leadpony
Copy link
Owner

leadpony commented Sep 3, 2019

Now I identified the error location.

@leadpony leadpony self-assigned this Sep 3, 2019
@toddobryan
Copy link
Author

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

@leadpony leadpony added the bug Something isn't working label Sep 3, 2019
@leadpony
Copy link
Owner

leadpony commented Sep 3, 2019

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

leadpony added a commit that referenced this issue Sep 4, 2019
@leadpony
Copy link
Owner

leadpony commented Sep 4, 2019

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
Copy link
Author

Will do. Thanks again! Also, this suite caught a mistake in Jackson Core for which I've submitted a pull request: FasterXML/jackson-core#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
Copy link
Author

Works like a charm now! Thank you!

@leadpony
Copy link
Owner

leadpony commented Sep 28, 2019

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

leadpony commented Oct 3, 2019

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
Copy link
Author

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
Copy link
Owner

leadpony commented Oct 4, 2019

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

@leadpony
Copy link
Owner

leadpony commented Oct 5, 2019

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

@leadpony leadpony closed this as completed Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants