Fix handling of bolt URL with port 80 #364
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
URLs are currently parsed in two places in the driver. First is before driver creation to determine if it should be a direct, routing or http driver based on the URL scheme. Then scheme is stripped off and rest of the code uses host and port string. Second time is before network connection is established. This time code parses host and port string, which is either the one used to create the driver or arrived in a routing procedure response. Scheme is missing in this case and dummy "http://" was attached just to make parser correctly handle the given string. Turned out "url-parse" library used for parsing removes port from the parsed URL if it is the default one for the scheme. So port 80 is removed when URL scheme is "http". This made driver always treat "localhost:80" as "localhost:7687" where 7687 is the default bolt port during the second parsing step. Default port 80 was removed and driver treated absence of the port as a sign to use the default bolt port. Same logic handles URLs like "bolt://localhost". Removal of the default port is problematic because driver can't know if no port was specified or default port was specified. Experimental HTTP and HTTPS support also suffers from the same problem. Default port for HTTP should be 7474 and not 80. This commit fixes the problem by using a different parsing library "uri-js" that always preserves the port. Dummy "http://" prefix is also changed to "none://" just in case.
To clarify that address being used for connection is not a URL but a host-port string.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
URLs are currently parsed in two places in the driver. First is before driver creation to determine if it should be a direct, routing or http driver based on the URL scheme. Then scheme is stripped off and rest of the code uses host and port string. Second time is before network connection is established. This time code parses host and port string, which is either the one used to create the driver or arrived in a routing procedure response. Scheme is missing in this case and dummy "http://" was attached just to make parser correctly handle the given string.
Turned out "url-parse" library used for parsing removes port from the parsed URL if it is the default one for the scheme. So port 80 is removed when URL scheme is "http". This made driver always treat "localhost:80" as "localhost:7687" where 7687 is the default bolt port during the second parsing step. Default port 80 was removed and driver treated absence of the port as a sign to use the default bolt port. Same logic handles URLs like "bolt://localhost". Removal of the default port is problematic because driver can't know if no port was specified or default port was specified. Experimental HTTP and HTTPS support also suffers from the same problem. Default port for HTTP should be 7474 and not 80.
This PR fixes the problem by using a different parsing library "uri-js" that always preserves the port. Dummy "http://" prefix is also changed to "none://" just in case.