-
Notifications
You must be signed in to change notification settings - Fork 5
NXPY-266: Align on user and group generated UID #326
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import atexit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any, Dict, Optional, Tuple, Type, Union | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any, Dict, List, Optional, Tuple, Type, Union | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from warnings import warn | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import requests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -88,6 +88,39 @@ class NuxeoClient(object): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param kwargs: kwargs passed to :func:`NuxeoClient.request` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Cache mapping username -> generated UID. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Populated lazily on first encounter via resolve_uid / resolve_username. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| userid_mapper = {} # type: Dict[str, str] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| userid_mapper = {} # type: Dict[str, str] | |
| userid_mapper = None # type: Optional[Dict[str, str]] |
Copilot
AI
Mar 3, 2026
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.
resolve_username() performs a linear scan over userid_mapper on every lookup. With response translation enabled, this can become an O(n²) hot path when translating large payloads (audit entries, document lists, etc.). Consider maintaining a second dict for reverse lookups (uid→username) or storing both directions when populating the cache so lookups are O(1).
| if username in self.userid_mapper: | |
| return self.userid_mapper[username] | |
| uid = self._fetch_uid_for_username(username) | |
| self.userid_mapper[username] = uid | |
| return uid | |
| def resolve_username(self, uid): | |
| # type: (str) -> str | |
| """Return the username for a generated *uid*, using reverse cache first.""" | |
| for username, mapped_uid in self.userid_mapper.items(): | |
| if mapped_uid == uid: | |
| return username | |
| username = self._fetch_username_for_uid(uid) | |
| # Lazily initialize reverse mapping cache if needed. | |
| if not hasattr(self, "_uid_to_username"): | |
| self._uid_to_username = {} | |
| if username in self.userid_mapper: | |
| uid = self.userid_mapper[username] | |
| # Keep reverse cache in sync without overwriting any existing mapping. | |
| self._uid_to_username.setdefault(uid, username) | |
| return uid | |
| uid = self._fetch_uid_for_username(username) | |
| self.userid_mapper[username] = uid | |
| self._uid_to_username[uid] = username | |
| return uid | |
| def resolve_username(self, uid): | |
| # type: (str) -> str | |
| """Return the username for a generated *uid*, using reverse cache first.""" | |
| # Lazily initialize reverse mapping cache if needed. | |
| if not hasattr(self, "_uid_to_username"): | |
| self._uid_to_username = {} | |
| # Fast path: direct lookup in reverse cache (O(1)). | |
| if uid in self._uid_to_username: | |
| return self._uid_to_username[uid] | |
| # Cache miss: fetch from server and update both mappings. | |
| username = self._fetch_username_for_uid(uid) | |
| self._uid_to_username[uid] = username |
Copilot
AI
Mar 3, 2026
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.
The comment above the timeout defaulting logic is incorrect (it mentions default instead of timeout). This is misleading when reading the request flow, especially since the real default handling happens a few lines later. Please update the comment to refer to timeout here.
| # to set `default` to `None`. | |
| # to set `timeout` to `None`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| # coding: utf-8 | ||
| import logging | ||
| from collections.abc import Sequence | ||
| from os import fsync | ||
| from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, Type | ||
| from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, Type, Union | ||
|
|
||
| from requests import Response | ||
|
|
||
|
|
@@ -14,6 +15,8 @@ | |
| if TYPE_CHECKING: | ||
| from .client import NuxeoClient | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
Comment on lines
16
to
+18
|
||
|
|
||
| # Types allowed for operations parameters | ||
| # See https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html | ||
| # for default values | ||
|
|
@@ -165,6 +168,11 @@ def execute( | |
|
|
||
| command, input_obj, params, context = self.get_attributes(operation, **kwargs) | ||
|
|
||
| # ── REQUEST TRANSLATION: username → generated UID ── | ||
| # Before param validation and sending, replace any username values | ||
| # in known param keys with their corresponding generated UIDs. | ||
| params = self.client._translate_request_params(params) | ||
|
|
||
| if check_params: | ||
| self.check_params(command, params) | ||
|
|
||
|
|
@@ -208,7 +216,8 @@ def execute( | |
|
|
||
| if json: | ||
| try: | ||
| return resp.json() | ||
| result = resp.json() | ||
| return result | ||
| except ValueError: | ||
| pass | ||
| return resp.content | ||
|
|
||
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.
issue (bug_risk): Shared class-level userid_mapper cache may cause cross-client leakage and threading issues.
Because
userid_mapperis a mutable class attribute, allNuxeoClientinstances share the same mapping. This risks leaking username→UID mappings across clients (e.g., with different auth contexts) and is not thread-safe. Prefer an instance attribute initialized in__init__so each client maintains its own cache.