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

Prevent NPE for relative fragment w/ base != null. #28

Merged
merged 3 commits into from
Mar 12, 2014
Merged

Prevent NPE for relative fragment w/ base != null. #28

merged 3 commits into from
Mar 12, 2014

Conversation

sideshowbarker
Copy link
Contributor

No description provided.

@smola
Copy link
Owner

smola commented Mar 12, 2014

@sideshowbarker Could you add a test case for this? You can try adding it to src/test/resources/data/urltestdata_whatwg.txt. If it's not reproducible there, you can add an ad-hoc test to src/test/java/io/mola/galimatias/URL2Test.java.

@sideshowbarker
Copy link
Contributor Author

@smola There's sorta already a test case for it at https://github.com/smola/galimatias/blob/master/src/test/resources/data/urltestdata_whatwg.txt#L32 That's the simplest case that will reproduce it -- just the string "#" (relative URL with no path part and no query part but with an (empty) fragment part).

I say that's only "sorta" the right test case because it will only trigger that NPE is you're parsing a string when the base URL is null. But your test harness runs all its test cases with a base URL of "http://example.org/foo/bar" (because that's what that test data assumes). So as far as I can see, the current test harness is never going to be able exercise that code path unless you introduce some testing for the case when base URL set to null (in which case of course all the relative-URL cases should just completely fail -- maybe with a "No base URL set." message).

By the way, if the base URL is null, parsing any string with a fragment part but no query part will trigger that NPE. So anything in lines 32-35 of https://github.com/smola/galimatias/blob/master/src/test/resources/data/urltestdata_whatwg.txt or "#foo" or whatever.

You can test this right now with the CLI by just doing this:

java -cp ./target/galimatias-0.0.3-SNAPSHOT.jar io.mola.galimatias.cli.CLI "#"

That will trigger the NPE in the current code, because the CLI currently just always tests with the base URL set to null. (So, unlike the test harness, the CLI does exercise the problem code.)

@sideshowbarker
Copy link
Contributor Author

Actually, looking at this further, it seems to me you'll hit this NPE even when the base URL is not null, if the following are both true:

  1. the string being parsed is a relative URL that has no query part or path part, and
  2. the base URL has no query part
    Notice that even after the 8202e94 change, the CLI still hits the NPE on "#" because the base URL that the CLI uses has no query part.

@sideshowbarker
Copy link
Contributor Author

Added a test to src/test/java/io/mola/galimatias/URL2Test.java. The test fails on the current trunk (without the fix for the NPE applied).

smola added a commit that referenced this pull request Mar 12, 2014
Prevent NPE for relative fragment w/ base != null.
@smola smola merged commit 5ffa96c into smola:master Mar 12, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 43bf6b8 on sideshowbarker:master into 4ce66dc on smola:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling cb2907d on sideshowbarker:master into 4ce66dc on smola:master.

@smola smola added the bug label May 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants