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

Url parsing: align parsing implementation with grammar #3244

Closed
jcamiel opened this issue Sep 19, 2024 · 5 comments · Fixed by #3364
Closed

Url parsing: align parsing implementation with grammar #3244

jcamiel opened this issue Sep 19, 2024 · 5 comments · Fixed by #3364
Labels
enhancement New feature or request
Milestone

Comments

@jcamiel
Copy link
Collaborator

jcamiel commented Sep 19, 2024

In the Hurl grammar, URL is parsed as:

request:
  lt*
  method sp value-string lt
  header*
  request-section*
  body?

Where value-string is a "common" token : in particular, as value-string, URL should accept Hurl unicode literals (ex: \u{7b}), or curly braces ({). Related to discussion #3243, we should accept the following Hurl file where the URL contains {and }without being URL encoded:

GET https://www.example.com/index.html?foo={bar}
HTTP 200
@jcamiel jcamiel added the enhancement New feature or request label Sep 19, 2024
@jcamiel jcamiel added this to the 6.0.0 milestone Sep 19, 2024
@jcamiel jcamiel changed the title Url parsing: align parsing implementation with gramar Url parsing: align parsing implementation with grammar Sep 19, 2024
@fabricereix
Copy link
Collaborator

@jcamiel @lepapareil
Related to using a standard Hurl string value for url, do we also apply the following changes?

  • http:// or https:// protocol prefix not checked anymore at parse-time
  • https:// will be added at runtime as a default

@jcamiel
Copy link
Collaborator Author

jcamiel commented Sep 29, 2024

Both seem OK! You can also do some "git blaming" on the function that checks the http/https prefix to see why we added it: I remember adding an integration test for it but don't remember what has triggered it...

@muffl0n
Copy link
Contributor

muffl0n commented Sep 29, 2024

Can't we just outsource the validation to curl? Just give it to curl and see if it accepts it.
After replacing the vars, of course.

@jcamiel
Copy link
Collaborator Author

jcamiel commented Sep 29, 2024

There are some logic regarding redirection where we must transform the value of the url to an URL structure so I think we can't simply accept any value and pass it to libcurl.

@fabricereix
Copy link
Collaborator

I think the http:// prefix check was simply here to fail as early as possible with nice error message.

@fabricereix fabricereix linked a pull request Nov 2, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants