-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
PYTHON-3096 Finish implementation and tests for GSSAPI options #1985
base: master
Are you sure you want to change the base?
Conversation
af, socktype, proto, canonname, sockaddr = socket.getaddrinfo( | ||
hostname, None, 0, 0, socket.IPPROTO_TCP, socket.AI_CANONNAME | ||
)[0] | ||
|
||
# For forward just to resolve the cname as dns.lookup() will not return it. | ||
if option == "forward": |
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.
It looks like the string "none"
is also valid:
def _validate_canonicalize_host_name(value: str | bool) -> str | bool:
valid_names = [False, True, "none", "forward", "forwardAndReverse"]
Should that have the same behavior as False
? Or should validate convert "none" to False?
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.
I changed it to convert "none" to false, so the three validated options are True, False, and "forward".
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.
Actually no, the spec test is expecting it to validate as a string, so I updated the business logic instead.
await client.server_info() | ||
|
||
if props["CANONICALIZE_HOST_NAME"] == "none": | ||
return |
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.
This if-statement should be removed since it's redundant, or is there something missing?
af, socktype, proto, canonname, sockaddr = socket.getaddrinfo( | ||
hostname, None, 0, 0, socket.IPPROTO_TCP, socket.AI_CANONNAME | ||
)[0] | ||
|
||
# For forward just to resolve the cname as dns.lookup() will not return it. | ||
if option == "forward": | ||
return canonname.lower() |
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.
I'm still confused by this method. DRIVERS-1803 says:
"forward" is equivalent to the current behaviour of true (a forward DNS lookup)
But here we are introducing new behavior for "forward". Is that because our behavior for "true" is actually "forwardAndReverse"? If so could you add a comment mentioning that?
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.
Ah, I misinterpreted the Node implementation and got confused. I'll redo the logic.
No description provided.