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

Updates Cookie class to be more generic in attribute parsing and emit. #521

Merged
merged 3 commits into from
May 29, 2020

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented May 22, 2020

What problem does this code solve?
See #517
Brings the Cookie implementation closer to the RFC specification (https://tools.ietf.org/html/rfc6265)
This is so the library can age better as new attributes are added or removed in future RFC revisions.

This also fixes bugs in the implementation:

  • Allowed empty cookie names which should be ignored
  • Attribute names were case sensitive when they should not be
  • Name and Value of the cookie were not ensured trimmed
  • Attributes were not ensured to have white space removed

Risks
Low to Medium. I doubt many people were using this since the problems brought up in #517 were not found until now. They seem to be pretty glaring gaps in the parsing and emit logic.

Changes to the API?
No

Will this require a new release?
Yes

Should the documentation be updated?
I don't think it's needed

Does it break the unit tests?
Yes, corrections were made

Was any code refactored in this commit?
No, bug fixes and support for new attributes only.

Review status
APPROVED

Starting 3 day comment window

@stleary
Copy link
Owner

stleary commented May 23, 2020

More discussion can be found in #517, which is now closed. Please continue the discussion here.

@johnjaylward johnjaylward changed the title Updates Cookie class to be a more generic in attribute parsing and emit. Updates Cookie class to be more generic in attribute parsing and emit. May 26, 2020
John J. Aylward added 2 commits May 26, 2020 08:53
This is so the library can age better as new attributes are added to RFC
revisions.
1. Made Cookie Name and Value properties case insensitive
2. Throws exception on illegal Cookie Name
3. Doesn't emit "false" flag values
4. Properly escape key-value attributes.
cookie-keys are not case sensitive, but json-keys are.
@johnjaylward
Copy link
Contributor Author

@Hubgut, please review the latest changes. I believe I corrected the escaping issue and also a few other minor tweeks

@stleary
Copy link
Owner

stleary commented May 27, 2020

Unit tests fail for Maven with Java 7 but pass for Gradle with Java 8. The failures are in unrelated modules, due to the unit tests not accounting for possible differences in JSONObject ordering.
JSONObjectTest, XMLConfigurationTest, XMLTest.
These failures will be fixed in a separate commit.

@stleary stleary merged commit fee6ddb into stleary:master May 29, 2020
@Hubgut
Copy link

Hubgut commented May 31, 2020

@johnjaylward. Now, I found some time for my little project and changed to the most current version.
I had no time for thorough testing yet; but it seems to be working!
I have also looked into the Cookies class. It looks much better now. My former complaints are gone.

@johnjaylward johnjaylward deleted the CookieFlagSupport branch June 1, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants