PYTHON-5488 append_metadata should not add duplicates
#2461
Conversation
pymongo/pool_options.py
Outdated
| if driver.name in self.__metadata["driver"]["name"].split("|"): | ||
| return |
There was a problem hiding this comment.
I'm thinking we should be case-insensitive. "LangChain" vs. "langchain" should not make a difference.
I cannot remember if we already lowercase everything in name (I don't think we do).
There was a problem hiding this comment.
good call, changed!
|
Another thought, should we be adding a |
pymongo/pool_options.py
Outdated
| @@ -386,6 +386,8 @@ def __init__( | |||
|
|
|||
| def _update_metadata(self, driver: DriverInfo) -> None: | |||
| """Updates the client's metadata""" | |||
| if driver.name.lower() in self.__metadata["driver"]["name"].lower().split("|"): | |||
There was a problem hiding this comment.
Need to check for driver.name being None before calling lower().
There was a problem hiding this comment.
the type of DriverInfo.name shouldn't be none. Would the check here be enforcing that the caller is following that rule?
There was a problem hiding this comment.
We check on line 393 that driver.name exists, so extending the same check here makes sense. We don't explicitly prevent driver.name from being None, so I'd prefer to err on the safe side to prevent errors being thrown by misbehaving users.
| @@ -210,6 +210,46 @@ async def test_doesnt_update_established_connections(self): | |||
| self.assertIsNone(self.handshake_req) | |||
| self.assertEqual(listener.event_count(ConnectionClosedEvent), 0) | |||
|
|
|||
| async def test_duplicate_driver_name_no_op(self): | |||
There was a problem hiding this comment.
Can we refactor check_metadata_added to support checking that the new metadata is the same as the old metadata? Most of this method is just duplicate code from check_metadata_added.
Can you give an example? |
|
If I add DriverInfo with no |
Good point. Always adding "|" to each field, even if it's not present, seems like a good fix. |
Good catch. Sounds like we need a spec update for this case to ensure all drivers behave the same. |
|
@sleepyStick we're still missing the logic to ensure an equal number of |
pymongo/pool_options.py
Outdated
| if driver.platform: | ||
| metadata["platform"] = "{}|{}".format(metadata["platform"], driver.platform) | ||
|
|
||
| name = driver.name if driver.name else "" |
There was a problem hiding this comment.
These can be simplified to driver.xxx or ""
|
drivers-pr-bot please backport to v4.14 |
| @@ -386,20 +386,26 @@ def __init__( | |||
|
|
|||
| def _update_metadata(self, driver: DriverInfo) -> None: | |||
| """Updates the client's metadata""" | |||
| if driver.name and driver.name.lower() in self.__metadata["driver"]["name"].lower().split( | |||
There was a problem hiding this comment.
Minor nit: we don't need the split() call here. Does having it improve readability?
There was a problem hiding this comment.
I was mainly worried about potential substrings? like if driver.name is a substring of an existing name, its not the same and therefore still should be added?
There was a problem hiding this comment.
Ah good point! Great find on that edge case, better to be safe here and use the split!
(not sure if this was urgent or not, but it was a relatively quick fix and i was worried this would be blocking the langchain PR)