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

splitpath and URL parameters #14

Closed
fonsp opened this issue Jan 4, 2021 · 4 comments · Fixed by #16
Closed

splitpath and URL parameters #14

fonsp opened this issue Jan 4, 2021 · 4 comments · Fixed by #16

Comments

@fonsp
Copy link
Member

fonsp commented Jan 4, 2021

  1. splitpath does not handle URL parameters correctly
  2. splitpath does not accept a URI
julia> URIs.splitpath("/a/b?c=d")
1-element Array{String,1}:
 "a"

julia> URIs.splitpath("/a/b")
2-element Array{String,1}:
 "a"
 "b"

julia> URIs.splitpath(URI("/a/b?c=d"))
ERROR: MethodError: no method matching splitpath(::URI)
Closest candidates are:
  splitpath(::AbstractString) at /Users/fons/.julia/packages/URIs/1jrj1/src/URIs.jl:382
Stacktrace:
 [1] top-level scope at REPL[9]:1

julia> URIs.splitpath(URI("/a/b?c=d").path)
2-element Array{String,1}:
 "a"
 "b"

URIs.jl v1.1.0

@fonsp
Copy link
Member Author

fonsp commented Jan 4, 2021

Calling splitpath directly on the request target is recommended in the HTTP docs: https://juliaweb.github.io/HTTP.jl/stable/public_interface/#HTTP.Handlers

@c42f
Copy link
Collaborator

c42f commented Jan 12, 2021

Does #16 fix this for you?

Does not handle URL parameters correctly

TBH I'm not quite sure we should expect it to split off the query and fragment here (that's handled by the URI parser). But it seems splitpath was originally designed to do this, and it's fairly harmless so I fixed that as well.

@fonsp
Copy link
Member Author

fonsp commented Jan 12, 2021

Awesome! That addresses all my problems.

I think splitting off the query is the right choice! To me, splitpath is to find out "where it is", analogously to Base.splitdir, and the query and fragment don't carry this information.

@c42f
Copy link
Collaborator

c42f commented Jan 13, 2021

To me, splitpath is to find out "where it is", analogously to Base.splitdir, and the query and fragment don't carry this information.

Yes definitely. But what I'm slightly unsure about here is why you'd have the path and query/fragment in str, but not just use parse(URI, str) to parse it as a relative URI, then use splitpath() from there — it seems a little odd to be doing extra parsing work in splitpath() when that's arguably the job of parse(URI, str) instead. (Also, splitpath() will never work on absolute URIs provided as strings.)

But anyway I can see that it might be convenient and it seems fairly harmless, so I think we can just go with dropping the query and fragment.

@c42f c42f closed this as completed in #16 Jan 13, 2021
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 a pull request may close this issue.

2 participants