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

Fix link parsing for non-trivial cases #2128

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

viliam-durina
Copy link

The previous implementation used naive parsing suffering from many issues, especially when special characters were part of values. It also didn't escape the special characters when serializing a link to string.

I marked the Link.valueOf method as deprecated because it doesn't correctly parse multiple links, nor does it handle multiple values for the rel param. One should use Links.parse. There are no other incompatible API changes.

I had to change one current test: we no longer ignore missing final > after the URL - such link is invalid anyway because it didn't have the rel parameter.

Fixes #2099

Tests have 100% coverage of the new code, except for one line that cannot currently happen, but is there for robustness.

@viliam-durina
Copy link
Author

@odrotbohm Hi, could you have a look at this PR? It fixes a serious issue and has 100% test coverage.

@odrotbohm
Copy link
Member

Given this adds significant complexity, would you mind extracting the actual parsing into a package protected type LinkParser? Link could then use that functionality from ….valueOf(…). It might make sense to move all (previously existing) parsing related tests into a dedicated unit test class solely focusing on that. Furthermore, please make sure to apply the formatters provided here and avoid unnecessary formatting changes (e.g. the removal of blank lines in the tests). Finally, please squash the individual commits into a single commit.

@viliam-durina
Copy link
Author

@odrotbohm I'll work on that. I have just one question: are the blank lines part of your standard format? I normally fix format of the code near which I'm working.

Viliam Durina added 5 commits June 27, 2024 16:08
The previous implementation used naive parsing suffering from many issues, especially when special characters were part of values. It also didn't escape the special characters when serializing a link to string.

I marked the `Link.valueOf` method as deprecated because it doesn't correctly parse multiple links, nor does it handle multiple values for the `rel` param. One should use `Links.parse`. There are no other incompatible API changes.

I had to change one current test: we no longer ignore missing final `>` after the URL - such link is invalid anyway because it didn't have the rel parameter.

Fixes spring-projects#2099
@viliam-durina
Copy link
Author

I pushed the requested changes, except:

  • I didn't move the parsing-related tests to a separate class. I test using the public API, which is the right thing to do IMO. And the public api is the Links class, not in the package-private LinkParser class. If you want to test against the internal API, or if you want to test the public API from a test class not strictly following the convention of appending UnitTest to the tested class name, let me know.

  • I'm using Intelij, but I don't seem to be able to import the formating rules in a jar. I ended importing the Eclipse XML rules to IntelliJ

  • I think the formatting after after auto-formatting the code is much less readable. Especially the line breaks in multi-line expressions are now placed randomly, probably after reaching a certain line length, and the result is much harder to read, especially the tests. Are you sure you want to apply the formatting? Could I get an exemption at least on the line breaks?

  • I rebased the branch (was quite stale), but I didn't squash yet. IMO squashing makes it much harder for reviewers to follow the changes. Furthermore, I hope you'll reconsider the formatting requirement, and I want to be able to revert that commit, which wouldn't be possible if I squashed them. There's a squash option when you'll be merging the PR, that's much more useful. You can even set github up so that a squash merge is the only option, we do that in all our projects and generally frown on force-pushes.

@odrotbohm
Copy link
Member

I'll take it from here.

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.

Link parsing only works for basic cases
2 participants