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.

Sign up for GitHub

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

Jump to bottom

Fix the absuri function to correctly handle relative location paths #13

Closed
wants to merge 1 commit into from
Closed

Fix the absuri function to correctly handle relative location paths #13

wants to merge 1 commit into from

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Nov 27, 2020 •
edited
Loading

This pull request fixes the absuri function to correctly handle relative location paths.

Fixes JuliaWeb/HTTP.jl#435
Fixes JuliaWeb/HTTP.jl#626

This was referenced Nov 27, 2020
Closed
Closed
@c42f
Copy link
Collaborator

c42f commented Nov 27, 2020

Thanks this seems useful. It'll need some documentation and tests, please :-) In particular https://tools.ietf.org/html/rfc7230#section-5.5 should be referenced somewhere here.

About the API — I think the constructor is not the best place to do this as it's not generic URI functionality. It seems to be an HTTP-specific concept.

Looking at the way you use this in https://github.com/JuliaWeb/HTTP.jl/pull/630/files, is the problem here that absuri is actually buggy, and doesn't do the right thing with the existing context argument?

@DilumAluthge
Copy link
Member Author

Looking at the way you use this in https://github.com/JuliaWeb/HTTP.jl/pull/630/files, is the problem here that absuri is actually buggy, and doesn't do the right thing with the existing context argument?

Ah, yes this might actually be the situation.

@c42f
Copy link
Collaborator

c42f commented Nov 27, 2020 •
edited
Loading

Ah, yes this might actually be the situation.

Unfortunately the design rationale for some of these things was lost as Sam's not around and I think he wrote most of this. But it seems absuri is specifically used in HTTP.jl to deal with redirects and Location header. So if that's what it's used for, we should probably just make it follow the appropriate RFC and document that it does so.

@DilumAluthge DilumAluthge marked this pull request as draft November 27, 2020 02:22
@DilumAluthge DilumAluthge changed the title Add the effective_request_uri kwarg to the URI and absuri functions Fix the absuri function to correctly handle relative location paths Nov 27, 2020
@DilumAluthge
Copy link
Member Author

You can ignore the HTTP-based tests that I just added. I'll remove them and replace them with proper unit tests before we convert this PR from draft PR to regular PR.

@DilumAluthge DilumAluthge marked this pull request as ready for review November 27, 2020 22:08
@DilumAluthge
Copy link
Member Author

DilumAluthge commented Nov 27, 2020 •
edited
Loading

Alright, take a look now.

I've removed the HTTP tests and I've added some unit tests.

I've also added references to the appropriate documentation.

c42f
c42f reviewed Nov 30, 2020
View reviewed changes
Copy link
Collaborator

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have a couple more queries about some detail. Especially what to do about the fragment? (Is this relevant for the Location header?)

test/absuri.jl
("https://en.wikipedia.org/api/rest_v1/page/summary/Potatoes", "Potato", "https://en.wikipedia.org/api/rest_v1/page/summary/Potato"),
("https://en.wikipedia.org/api/rest_v1/page/summary/Potatoes", "./Potato", "https://en.wikipedia.org/api/rest_v1/page/summary/Potato"),
("https://en.wikipedia.org/api/rest_v1/page/summary/Potatoes", "/foo/bar/baz", "https://en.wikipedia.org/foo/bar/baz"),
("https://en.wikipedia.org/api/rest_v1/page/summary/Potatoes", "https://julialang.org/foo/bar/baz", "https://julialang.org/foo/bar/baz"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Let's add a test for query handling` as well?

src/URIs.jl
path = String(uristring(normpath(joinpath(URI(; path = context.path), "..", String(uri.path)))))
end

return URI(context; path=path, query=uri.query)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's meant to happen to fragment here?

src/URIs.jl
# 2. https://web.archive.org/web/20200926022629/https://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-17.html
# 3. https://www.rfc-editor.org/rfc/rfc3986.html#section-4.2
# 4. https://web.archive.org/web/20201106144353/https://www.rfc-editor.org/rfc/rfc3986.html#section-4.2
# 5. https://www.rfc-editor.org/rfc/rfc3986.html#section-5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the official reference https://tools.ietf.org/html/rfc3986?

@Trashtalk217
Copy link

Trashtalk217 commented Mar 12, 2021 •
edited
Loading

I've been doing some local testing with this branch (since I needed the feature), and I believe I found a bug. Adding a relative URI to an absolute one seems to give an invalid URI. Namely:

julia> test = absuri("contents/introduction/introduction.html", "https://algorithm-archive.org/")
URI("https://algorithm-archive.org/contents/introduction/introduction.html")

julia> test
URI("https://algorithm-archive.org/contents/introduction/introduction.html")

julia> test.uri
""

The .uri field is empty and I don't think it should be. Correct me if I'm wrong, of course, but how could this be fixed?

@DilumAluthge
Copy link
Member Author

Unfortunately, I don't have the time right now to fix the bugs and finish up this pull request.

Perhaps someone else would be willing to pick up the reins? Please feel free to take this code and use it as a starting point!

@DilumAluthge DilumAluthge deleted the dpa/relative-uris branch March 16, 2021 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c42f c42f c42f left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Julia doesn't like Potatoes HTTP GET error after get 302 response
3 participants
@DilumAluthge @c42f @Trashtalk217