Skip to content

Conversation

last-genius
Copy link
Contributor

@last-genius last-genius commented Jun 25, 2024

Removed unused function, replaced the only other usage of parse_uri with the
functions from the Uri module.

Suggested here.

The original ticket also mentioned authorization_of_string as another
candidate to switch to Uri, but it parses headers, not Uris, so this
was not undertaken.

This change passed the BST/BVT test suites, and was also tested
manually by interacting with the host from XenCenter.

(* Request-Line = Method SP Request-URI SP HTTP-Version CRLF *)
let uri, query = Http.parse_uri uri in
let uri_t = Uri.of_string uri in
let uri = Uri.path uri_t in
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this uri the same as the one on L368? If so, why converting it back?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would trust Uri to create standard-conforming URLs. One caveat though and why this needs testing:

utop # let t = Uri.of_string "https://foo.bar" in Uri.path t;;
- : string = ""

This might be surprising and you could have expected "/".

Copy link
Contributor Author

@last-genius last-genius Jun 25, 2024

Choose a reason for hiding this comment

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

Is this uri the same as the one on L368? If so, why converting it back?

L368's uri is the full path+query.
Here it's split into only the path, L381 further gets only the query part.

Couldn't come up with a more precise name so I just shadowed the other (as it was done before)

for example:

utop # let t = Uri.of_string "https://foo.bar/dir1/dir2?k1=v1" in Uri.path t;;
- : string = "/dir1/dir2"-
utop # let t = Uri.of_string "https://foo.bar/dir1/dir2?k1=v1" in Uri.query t;;
- : (string * string list) list = [("k1", ["v1"])]

@last-genius last-genius force-pushed the private/asultanov/uri-improvement branch from 453a911 to 7b97685 Compare June 25, 2024 14:56
@last-genius last-genius marked this pull request as draft June 25, 2024 14:58
@last-genius last-genius force-pushed the private/asultanov/uri-improvement branch 4 times, most recently from 059b9de to 8d92fcd Compare June 25, 2024 15:31
bernhardkaindl added a commit that referenced this pull request Jun 26, 2024
…ix-ocaml-ci-pr-5726

Merge master into feature/py3 to fix OCaml CI failures using #5726
@last-genius last-genius force-pushed the private/asultanov/uri-improvement branch from 8d92fcd to de02e53 Compare June 26, 2024 13:17
@last-genius
Copy link
Contributor Author

last-genius commented Jun 26, 2024

Created a separate ticket (CP-50079) for removing parse_keyvalpairs, as there are more questions and complexities there (to discuss at the next standup, should also discuss if more tests are needed here already, or in the new ticket).

Restored the exception handling over Uri's interface - could someone review this please? (it's one line - L377 in http_svr.ml)

@lindig lindig marked this pull request as ready for review June 27, 2024 08:44
@lindig lindig merged commit 31eeb93 into xapi-project:master Jul 1, 2024
@last-genius last-genius deleted the private/asultanov/uri-improvement branch July 2, 2024 08:11
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.

5 participants