NXPY-266: Align on user and group generated UID#326
Open
gitofanindya wants to merge 1 commit intomasterfrom
Open
NXPY-266: Align on user and group generated UID#326gitofanindya wants to merge 1 commit intomasterfrom
gitofanindya wants to merge 1 commit intomasterfrom
Conversation
Contributor
Reviewer's GuideIntroduce a username ↔ generated-UID translation layer in NuxeoClient and wire it into both low-level HTTP requests and automation operations, with comprehensive unit tests validating request/response mapping and cache behavior. Sequence diagram for request/response username_UID translation in operationssequenceDiagram
actor Caller
participant Ops as NuxeoOperations
participant Client as NuxeoClient
participant Session as RequestsSession
participant Resp as Response
Caller->>Ops: execute(operation, params={userId: "alice"}, json=True)
Ops->>Ops: get_attributes(operation, **kwargs)
Ops->>Client: _translate_request_params(params)
activate Client
Client->>Client: _translate_single_username_to_uid("alice")
Client->>Client: resolve_uid("alice")
alt uid cached
Client-->>Ops: params with userId="uid-alice"
else uid not cached
Client->>Client: _fetch_uid_for_username("alice")
Client->>Client: userid_mapper["alice"] = "uid-alice"
Client-->>Ops: params with userId="uid-alice"
end
deactivate Client
Ops->>Ops: check_params(command, translated_params)
Ops->>Client: request(method, path, params=translated_params, **kwargs)
activate Client
Client->>Client: _translate_request_params(kwargs["params"])
Client->>Session: send HTTP request
Session-->>Client: Resp
Client->>Client: wrap Resp.json()
Client-->>Ops: Resp
deactivate Client
Ops->>Resp: json()
activate Resp
Resp-->>Ops: data_with_usernames
deactivate Resp
Ops-->>Caller: data_with_usernames
Class diagram for NuxeoClient username_UID mapping and translationclassDiagram
class NuxeoClient {
+Dict~str,str~ userid_mapper
+frozenset _USERNAME_REQUEST_KEYS
+frozenset _USERNAME_RESPONSE_KEYS
+__init__(auth, host, scheme, proxies, verify, timeout, headers, client_kwargs)
+disable_retry()
+query(query, **kwargs)
+request(method, path, **kwargs)
+_fetch_uid_for_username(username: str) str
+_fetch_username_for_uid(uid: str) str
+resolve_uid(username: str) str
+resolve_username(uid: str) str
+_translate_request_params(params: Dict~str,Any~) Dict~str,Any~
+_translate_single_username_to_uid(value: str) str
+_translate_response(data: Any) Any
+_translate_uid_value_to_username(value: Any) Any
}
class NuxeoOperations {
+client: NuxeoClient
+execute(operation, input_obj, params, context, check_params, json, **kwargs)
+get_attributes(operation, **kwargs)
+check_params(command, params)
}
NuxeoOperations --> NuxeoClient : holds_reference
NuxeoOperations ..> NuxeoClient : calls _translate_request_params
NuxeoClient ..> NuxeoClient : resolve_uid
NuxeoClient ..> NuxeoClient : resolve_username
NuxeoClient ..> NuxeoClient : _translate_request_params
NuxeoClient ..> NuxeoClient : _translate_response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider making
userid_mapperan instance attribute initialized in__init__instead of a class attribute, otherwise allNuxeoClientinstances will share a global user→UID cache which may be surprising in multi-tenant or test scenarios. resolve_usernamedoes a linear scan overuserid_mappervalues on every cache miss; if this mapping can grow large or be heavily used, it may be worth maintaining a reverseuid -> usernameindex to avoid O(n) lookups.- The updated comment in
requestnow says "Set the default value toobjectto allow someone to setdefaulttoNone.", but the code still concerns thetimeoutparameter; it would be clearer to keep the wording consistent with the actual argument name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `userid_mapper` an instance attribute initialized in `__init__` instead of a class attribute, otherwise all `NuxeoClient` instances will share a global user→UID cache which may be surprising in multi-tenant or test scenarios.
- `resolve_username` does a linear scan over `userid_mapper` values on every cache miss; if this mapping can grow large or be heavily used, it may be worth maintaining a reverse `uid -> username` index to avoid O(n) lookups.
- The updated comment in `request` now says "Set the default value to `object` to allow someone to set `default` to `None`.", but the code still concerns the `timeout` parameter; it would be clearer to keep the wording consistent with the actual argument name.
## Individual Comments
### Comment 1
<location> `nuxeo/client.py:93` </location>
<code_context>
+ # Cache mapping username -> generated UID.
+ # Populated lazily on first encounter via resolve_uid / resolve_username.
+ userid_mapper = {} # type: Dict[str, str]
+
+ # --- Known param keys that carry a username value in REQUESTS ---
</code_context>
<issue_to_address>
**issue (bug_risk):** Shared class-level userid_mapper cache may cause cross-client leakage and threading issues.
Because `userid_mapper` is a mutable class attribute, all `NuxeoClient` instances 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.
</issue_to_address>
### Comment 2
<location> `tests/unit/test_client_userid_mapper.py:499-508` </location>
<code_context>
+ def test_response_is_translated(self):
</code_context>
<issue_to_address>
**issue (testing):** This test name and docstring suggest UID→username translation, but the assertions only validate that `execute()` calls `resp.json()`.
Since `client.request` is mocked at the API level, the `NuxeoClient.request()` wrapper (which patches `resp.json` to call `_translate_response`) isn’t exercised. The test only checks that `execute()` returns the mock JSON, not that translation occurs. Either rename the test/docstring to match this behavior, or use a real `NuxeoClient.request()` (as in `TestRequestIntegration`) so you can assert on translated fields (e.g. `lockOwner == 'admin'`).
</issue_to_address>
### Comment 3
<location> `tests/unit/test_client_userid_mapper.py:267-273` </location>
<code_context>
+ result = client._translate_request_params(params)
+ assert result["delegatedActors"] == ["user:uid-carol"]
+
+ def test_users_key_scalar(self):
+ client = _make_client()
+ params = {"users": "alice"}
+ result = client._translate_request_params(params)
+ assert result["users"] == "uid-alice"
+
+ def test_non_string_non_list_value_passthrough(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Missing coverage for list-valued `"users"` (and similar) params in `_translate_request_params`.
You already test scalar `"users"` and list-valued `"actors"` / `"delegatedActors"`, but `_translate_request_params` supports list values for all `_USERNAME_REQUEST_KEYS`. Please add coverage for list-valued `"users"`, e.g.:
```python
params = {"users": ["alice", "bob"]}
result = client._translate_request_params(params)
assert result["users"] == ["uid-alice", "uid-bob"]
```
Optionally, add a similar list-valued test for `"user"` to verify consistent behavior across all username keys.
```suggestion
def test_users_key_scalar(self):
client = _make_client()
params = {"users": "alice"}
result = client._translate_request_params(params)
assert result["users"] == "uid-alice"
def test_users_key_list(self):
client = _make_client()
params = {"users": ["alice", "bob"]}
result = client._translate_request_params(params)
assert result["users"] == ["uid-alice", "uid-bob"]
def test_user_key_list(self):
client = _make_client()
params = {"user": ["alice", "bob"]}
result = client._translate_request_params(params)
assert result["user"] == ["uid-alice", "uid-bob"]
def test_non_string_non_list_value_passthrough(self):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| # Cache mapping username -> generated UID. | ||
| # Populated lazily on first encounter via resolve_uid / resolve_username. | ||
| userid_mapper = {} # type: Dict[str, str] |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Shared class-level userid_mapper cache may cause cross-client leakage and threading issues.
Because userid_mapper is a mutable class attribute, all NuxeoClient instances 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.
Comment on lines
+499
to
+508
| def test_response_is_translated(self): | ||
| """Response JSON with known keys is translated by resp.json() wrapper.""" | ||
| api, client = _make_api() | ||
| client.userid_mapper["admin"] = "uid-admin" | ||
|
|
||
| mock_response = MagicMock() | ||
| mock_response.json.return_value = { | ||
| "entity-type": "document", | ||
| "uid": "doc-1", | ||
| "lockOwner": "uid-admin", |
Contributor
There was a problem hiding this comment.
issue (testing): This test name and docstring suggest UID→username translation, but the assertions only validate that execute() calls resp.json().
Since client.request is mocked at the API level, the NuxeoClient.request() wrapper (which patches resp.json to call _translate_response) isn’t exercised. The test only checks that execute() returns the mock JSON, not that translation occurs. Either rename the test/docstring to match this behavior, or use a real NuxeoClient.request() (as in TestRequestIntegration) so you can assert on translated fields (e.g. lockOwner == 'admin').
Comment on lines
+267
to
+273
| def test_users_key_scalar(self): | ||
| client = _make_client() | ||
| params = {"users": "alice"} | ||
| result = client._translate_request_params(params) | ||
| assert result["users"] == "uid-alice" | ||
|
|
||
| def test_non_string_non_list_value_passthrough(self): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing coverage for list-valued "users" (and similar) params in _translate_request_params.
You already test scalar "users" and list-valued "actors" / "delegatedActors", but _translate_request_params supports list values for all _USERNAME_REQUEST_KEYS. Please add coverage for list-valued "users", e.g.:
params = {"users": ["alice", "bob"]}
result = client._translate_request_params(params)
assert result["users"] == ["uid-alice", "uid-bob"]Optionally, add a similar list-valued test for "user" to verify consistent behavior across all username keys.
Suggested change
| def test_users_key_scalar(self): | |
| client = _make_client() | |
| params = {"users": "alice"} | |
| result = client._translate_request_params(params) | |
| assert result["users"] == "uid-alice" | |
| def test_non_string_non_list_value_passthrough(self): | |
| def test_users_key_scalar(self): | |
| client = _make_client() | |
| params = {"users": "alice"} | |
| result = client._translate_request_params(params) | |
| assert result["users"] == "uid-alice" | |
| def test_users_key_list(self): | |
| client = _make_client() | |
| params = {"users": ["alice", "bob"]} | |
| result = client._translate_request_params(params) | |
| assert result["users"] == ["uid-alice", "uid-bob"] | |
| def test_user_key_list(self): | |
| client = _make_client() | |
| params = {"user": ["alice", "bob"]} | |
| result = client._translate_request_params(params) | |
| assert result["user"] == ["uid-alice", "uid-bob"] | |
| def test_non_string_non_list_value_passthrough(self): |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by Sourcery
Introduce a username↔generated-UID translation layer in the Nuxeo Python client so requests send generated UIDs while responses expose usernames transparently.
Enhancements:
Tests: