feat: add sanitized_url property to parse parse result #434
Conversation
zealws
left a comment
zealws
left a comment
There was a problem hiding this comment.
I'm happy with this approach, instead of using the pip internal function. Minor suggestion for the implementation though.
| @property | ||
| def sanitized_url(self) -> str: | ||
| """Sanitized DB URL with the password masked""" | ||
| return f"{self.scheme}://{self.username}:****@{self.hostname}:{self.port}/{self.database}/{self.schema}" |
There was a problem hiding this comment.
You should use one of the urllib functions to put the URL back together, since it handles quoting if necessary.
Also, if the password is empty, I think we should omit the :****. This would help with debugging if the password is omitted from the job, which can happen if it's not configured properly.
not sure which of these is better for this use-case:
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlunparse
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlunsplit
There was a problem hiding this comment.
@jarbesfeld said he could help pick this up while I switch to a different project
| @property | ||
| def sanitized_url(self) -> str: | ||
| """Sanitized DB URL with the password masked""" | ||
| return urlunparse( | ||
| ( | ||
| self.scheme, | ||
| self.username, | ||
| self.hostname, | ||
| self.port, | ||
| self.database, | ||
| self.schema, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
You really need to put some unit-tests on this, cause this won't work at all.
>>> url
'postgresql://uta_admin:localdev@localhost:5432/uta/uta_20241220'
>>> r = ParseResult(urlparse(url))
>>> r
ParseResult(scheme='postgresql', netloc='uta_admin:localdev@localhost:5432', path='/uta/uta_20241220', params='', query='', fragment='')
>>> r.sanitized_url
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
import platform
File "/Users/zxw016/dev/wagner/cool-seq-tool/src/cool_seq_tool/sources/uta_database.py", line 961, in sanitized_url
return urlunparse(
(
...<6 lines>...
)
)
File "/Users/zxw016/.pyenv/versions/3.13.5/lib/python3.13/urllib/parse.py", line 531, in urlunparse
_coerce_args(*components))
~~~~~~~~~~~~^^^^^^^^^^^^^
File "/Users/zxw016/.pyenv/versions/3.13.5/lib/python3.13/urllib/parse.py", line 130, in _coerce_args
raise TypeError("Cannot mix str and non-str arguments")
TypeError: Cannot mix str and non-str arguments
Notes:
- We want to preserve the
****in the URL, but only if the password was non-empty to begin with. - You need to reconstruct a named tuple that looks like the return value of
urlparse. - The port is an integer, which is what causes the exception above. This is fixed by using a named tuple.
- The
databaseandschemashouldn't be embedded in the URL directly. They were parsed fromself.pathand so you should use theself.pathinstead.
| sanitized_url = redact_auth_from_url(UTA_DB_URL) | ||
| try: | ||
| parsed_result = ParseResult(urlparse(db_url)) | ||
| sanitized_url = parsed_result.sanitized_url |
There was a problem hiding this comment.
Also, this line has to be outside the try, otherwise it's not in-scope for the except statements.
zealws
left a comment
zealws
left a comment
There was a problem hiding this comment.
Some minor suggestions, but it looks good to me otherwise!
| netloc = "" | ||
| if self.username: | ||
| netloc += self.username | ||
| if self.password is not None and self.password != "": |
There was a problem hiding this comment.
I don't think the self.password will ever be None, judging from the docs...
But if self.password: catches both None and "" (they're both false-y) anyway.
| ( | ||
| self.scheme, | ||
| netloc, | ||
| self.path, | ||
| self.params, | ||
| self.query, | ||
| self.fragment, | ||
| ) |
There was a problem hiding this comment.
I would really prefer we use a named tuple here, cause it's not obvious what order these should be in.
| ( | |
| self.scheme, | |
| netloc, | |
| self.path, | |
| self.params, | |
| self.query, | |
| self.fragment, | |
| ) | |
| UrlLibParseResult( | |
| scheme=self.scheme, | |
| netloc=netloc, | |
| path=self.path, | |
| params=self.params, | |
| query=self.query, | |
| fragment=self.fragment, | |
| ) |
No description provided.