-
Notifications
You must be signed in to change notification settings - Fork 94
Improved HttpConnection#buildBaseUrl to allow usage of Paths #386
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
Conversation
Hi @Likqez ,
can you please provide a specific example or a unit test to clarify the need for such changes?
Because requests are HTTP based they should also behave that way.
The other arango drivers are also mostly working with URLs and therefor already supporting paths.
The java driver should also support "normal" URL like requests.
Using the unchanged function, the port would be added after the path
example.com/arango:443 instead of example.com:443/arango
My changes only fixes the wrong URL construction. Even if it's an unintentional use case.
Any updates on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree with the idea of supporting url paths, the proposed approach is a bit confusing from an API standpoint, since it splits the host string to insert the port into it.
A cleaner approach could be introducing a new configuration property, i.e. named urlPath, and appending it to the url string.
URL paths support has also impact on parsing dynamically acquired hosts endpoints, triggered by acquireHostList settings.
But unfortunately URL paths are not officially supported by ArangoDB, as you can see in the documentation related to endpoints name format:
- https://www.arangodb.com/docs/3.7/http/endpoints.html#get-information-about-all-coordinator-endpoints
- https://www.arangodb.com/docs/3.7/programs-arangod-cluster.html#my-advertised-endpoint
Even if it works for some db versions, this is not intentionally allowed, not tested and the db behavior could change in the future.
Therefore I would close this for now and reconsider it in case the db will officially support URL paths.
This PR changes the behavior on how the base URL for the HttpConnection was constructed.
Now possible:
example.com/arango