Patch Sample and Field endpoints #6411
Conversation
WalkthroughImplements content-type aware PATCH handling for samples (JSON and JSON-Patch), adds a registry-based JSON transform and RFC 6902 JSON Patch utilities, introduces SampleField for list-element patching, renames exported sample routes to SampleRoutes, adds a jsonpatch dependency, and expands unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Routes as SampleRoutes
participant Sample as Sample
participant JT as JSON Transform
participant JP as JSON Patch
Client->>Routes: PATCH /dataset/{d}/sample/{s}
Routes->>Sample: forward request
alt Content-Type: application/json
Sample->>JT: transform_json(field values)
JT-->>Sample: transformed values or error
alt transform errors
Sample-->>Client: 400 Bad Request (details)
else
Sample-->>Client: 200 OK (updated sample)
end
else Content-Type: application/json-patch+json
Sample->>JP: parse(patches, transform_fn=transform_json)
JP-->>Sample: Patch[] or error
alt parse/apply errors
Sample-->>Client: 400 Bad Request (details)
else
Sample-->>Client: 200 OK (updated sample)
end
else
Sample-->>Client: 415 Unsupported Media Type
end
sequenceDiagram
autonumber
actor Client
participant Routes as SampleRoutes
participant Field as SampleField
participant JP as JSON Patch
participant JT as JSON Transform
Client->>Routes: PATCH /dataset/{d}/sample/{s}/{field_path}/{field_id}
Routes->>Field: dispatch request
Field->>Field: validate dataset/sample/field is list, locate element
alt invalid target
Field-->>Client: 404 or 400 error
else
Field->>JP: parse(patches, transform_fn=transform_json)
JP-->>Field: Patch[] or error
alt parse/apply errors
Field-->>Client: 400 Bad Request (details)
else
Field-->>Client: 200 OK (patched field)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
fiftyone/server/utils/json_transform/types.py (2)
1-6: Fix docstring typo“registery” → “registry”. Also consider a clearer title like “JSON transform registry”.
-"""Json types registery. +"""JSON transform registry.
7-38: Prefer Mapping over dict for transformer inputsThese functions don’t mutate
value. Usingtyping.Mapping[str, Any]improves intent and flexibility.-from fiftyone.server.utils.json_transform.transform import register +from fiftyone.server.utils.json_transform.transform import register +import typing as t import fiftyone.core.labels as fol @@ -@register(fol.Classification) -def transform_classification(value: dict) -> fol.Classification: +@register(fol.Classification) +def transform_classification(value: t.Mapping[str, t.Any]) -> fol.Classification: @@ -@register(fol.Classifications) -def transform_classifications(value: dict) -> fol.Classifications: +@register(fol.Classifications) +def transform_classifications(value: t.Mapping[str, t.Any]) -> fol.Classifications: @@ -@register(fol.Detection) -def transform_detection(value: dict) -> fol.Detection: +@register(fol.Detection) +def transform_detection(value: t.Mapping[str, t.Any]) -> fol.Detection: @@ -@register(fol.Detections) -def transform_detections(value: dict) -> fol.Detections: +@register(fol.Detections) +def transform_detections(value: t.Mapping[str, t.Any]) -> fol.Detections: @@ -@register(fol.Polyline) -def transform_polyline(value: dict) -> fol.Polyline: +@register(fol.Polyline) +def transform_polyline(value: t.Mapping[str, t.Any]) -> fol.Polyline: @@ -@register(fol.Polylines) -def transform_polylines(value: dict) -> fol.Polylines: +@register(fol.Polylines) +def transform_polylines(value: t.Mapping[str, t.Any]) -> fol.Polylines:tests/unittests/server/utils/jsonpatch/test_json_patch_patch.py (1)
30-33: Clarify test docstringThis parametrizes all patch helper classes, not just Add.
- """Tests that Add helper class works as expected.""" + """Tests that patch helper classes delegate apply() to methods.*."""fiftyone/server/utils/json_transform/__init__.py (1)
9-11: Export surface: add all for clarityConsider declaring explicit exports.
import fiftyone.server.utils.json_transform.types # auto-register resource types from fiftyone.server.utils.json_transform.transform import transform + +__all__ = ["transform"]tests/unittests/server/utils/jsonpatch/test_jsonpatch_methods.py (2)
243-253: Docstring mismatch with test behaviorDocstring says it expects a ValueError, but the test asserts a successful append. Update the docstring to reflect behavior.
88-96: Test name vs expected exceptionThe name “test_value_err” expects ValueError, but the assertion checks AttributeError. Consider renaming for clarity or adjusting the expected exception based on the implementation.
fiftyone/server/routes/sample.py (3)
122-122: Type hint mismatch: _handle_patch returns Sample, not dictThe method returns a Sample instance that is later serialized. Update the annotation for correctness.
- def _handle_patch(self, sample: fo.Sample, data: dict) -> dict: + def _handle_patch(self, sample: fo.Sample, data: dict) -> fo.Sample: @@ - return sample + return sampleAlso applies to: 139-139
144-156: SampleField.patch payload type should be a JSON Patch listThe payload is a list of operations. Update the signature and docstring to reflect this.
- async def patch(self, request: Request, data: dict) -> dict: + async def patch(self, request: Request, data: List[dict]) -> dict: @@ - data: patch of type op, path, value. + data: a list of JSON Patch operations (objects with op, path, value/from).
171-179: Prefer precise exception handling for field lookupCatching Exception swallows unrelated errors. Limit to AttributeError/KeyError and include the original message in the 404 for easier debugging.
- try: - field_list = sample.get_field(path) - except Exception as e: + try: + field_list = sample.get_field(path) + except (AttributeError, KeyError): raise HTTPException( status_code=404, detail=f"Field '{path}' not found in sample '{sample_id}'", )fiftyone/server/utils/jsonpatch/patch.py (1)
55-57: Minor doc fixGrammar: “to an object”.
- def apply(self, src: Any) -> Any: - """Applies the patch operation an object. + def apply(self, src: Any) -> Any: + """Applies the patch operation to an object.tests/unittests/sample_route_tests.py (1)
445-455: Clarify Content-Type for SampleField patch helperThese requests carry JSON Patch arrays. Consider using "application/json-patch+json" for clarity and parity with Sample route tests.
- mock_request.headers = {"Content-Type": "application/json"} + mock_request.headers = {"Content-Type": "application/json-patch+json"}fiftyone/server/utils/jsonpatch/methods.py (2)
49-76: Make list handling explicit; avoid relying on error message stringsRelying on the text of TypeError is brittle. Handle lists/tuples explicitly before generic getitem.
- for name in pointer.parts: - try: - value = getattr(value, name) - continue - except AttributeError as attr_err: - if hasattr(value, "__getitem__"): - try: - value = value[name] - continue - except TypeError as err: - if "list indices must be integers or slices" in str( - err - ): - idx = int(name) - - if not 0 <= idx <= len(value): - raise IndexError( - "List index out of range" - ) from err - - value = value[idx] - continue - - raise attr_err + for name in pointer.parts: + try: + value = getattr(value, name) + continue + except AttributeError as attr_err: + # Lists/tuples: index by integer + if isinstance(value, (list, tuple)): + idx = int(name) + if not 0 <= idx < len(value): + raise IndexError("List index out of range") from attr_err + value = value[idx] + continue + # Mappings and other indexables + if hasattr(value, "__getitem__"): + value = value[name] + continue + raise attr_err
178-183: Copy should deep-copy to avoid aliasingShallow copying can create shared references (mutable objects). Deep-copy the value before adding.
+ import copy @@ - value = get(src, from_pointer) + value = copy.deepcopy(get(src, from_pointer)) add(src, pointer, value)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
fiftyone/server/routes/__init__.py(2 hunks)fiftyone/server/routes/sample.py(3 hunks)fiftyone/server/utils/__init__.py(1 hunks)fiftyone/server/utils/json_transform/__init__.py(1 hunks)fiftyone/server/utils/json_transform/transform.py(1 hunks)fiftyone/server/utils/json_transform/types.py(1 hunks)fiftyone/server/utils/jsonpatch/__init__.py(1 hunks)fiftyone/server/utils/jsonpatch/methods.py(1 hunks)fiftyone/server/utils/jsonpatch/patch.py(1 hunks)setup.py(1 hunks)tests/unittests/sample_route_tests.py(7 hunks)tests/unittests/server/utils/jsonpatch/test_json_patch_patch.py(1 hunks)tests/unittests/server/utils/jsonpatch/test_jsonpatch.py(1 hunks)tests/unittests/server/utils/jsonpatch/test_jsonpatch_methods.py(1 hunks)
🔇 Additional comments (4)
fiftyone/server/utils/__init__.py (1)
18-20: LGTM: alias and auto‑registrationImport alias
transform_jsonis clear and triggers type registration via package init.setup.py (1)
47-47: Bound or pin jsonpatch and verify it’s neededUnpinned dependency increases supply‑chain and breakage risk. Also, code in this PR uses internal
fiftyone.server.utils.jsonpatch, not the external package; confirm necessity.
- If needed: add a tested floor and an upper bound (e.g., <2) consistent with semantic versioning.
- If unused: remove to reduce attack surface.
Run to confirm usage of the external package:
Optionally check latest version and Python 3.12 support:
fiftyone/server/routes/__init__.py (1)
20-33: Route aggregation change looks good; please confirm coverage/back-compatImporting and composing SampleRoutes aligns with the new routing structure.
- Verify SampleRoutes includes all previously exposed sample endpoints (including per-field subroutes) and that no duplicate legacy tuples remain that could shadow these paths.
tests/unittests/server/utils/jsonpatch/test_jsonpatch.py (1)
15-29: Solid coverage of jsonpatch.parseTests are thorough and exercise key paths (fields, op support, transform_fn, batches).
Optional: add an edge-case asserting TypeError for non-dict, non-iterable inputs (e.g., an int) and for a bare string to ensure robust type checks.
Also applies to: 30-84, 85-176
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fiftyone/server/utils/jsonpatch/__init__.py(1 hunks)fiftyone/server/utils/jsonpatch/methods.py(1 hunks)fiftyone/server/utils/jsonpatch/patch.py(1 hunks)tests/unittests/server/utils/jsonpatch/test_jsonpatch_methods.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fiftyone/server/utils/jsonpatch/init.py
- fiftyone/server/utils/jsonpatch/patch.py
| name = pointer.parts[-1] | ||
|
|
||
| try: | ||
| if hasattr(target, "__getitem__"): |
There was a problem hiding this comment.
[nit] shared logic with get
There was a problem hiding this comment.
Yep you're right, that particular value should actually be __setitem__.
If you're talking more about the logic: get, set, and del all follow a similar pattern (of checking what the source object can do), but are just different enough that I thought abstracting it would make it more confusing and harder to work on.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fiftyone/server/routes/sample.py (1)
68-81: Abort on first failing JSON Patch opWe still accumulate errors and keep applying subsequent patch operations. RFC6902 requires stopping at the first failure to avoid partially mutated targets. Raise the 400 immediately when
p.applythrows instead of continuing.- errors = {} - for i, p in enumerate(patches): - try: - p.apply(target) - except Exception as e: - logger.error("Error applying patch %s: %s", p, e) - errors[str(patch_list[i])] = str(e) - - if errors: - raise HTTPException( - status_code=400, - detail=errors, - ) + for i, p in enumerate(patches): + try: + p.apply(target) + except Exception as e: + logger.error("Error applying patch %s: %s", patch_list[i], e) + raise HTTPException( + status_code=400, + detail={str(patch_list[i]): str(e)}, + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/server/routes/sample.py(3 hunks)fiftyone/server/utils/json_transform/transform.py(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/unittests/server/utils/jsonpatch/test_jsonpatch_methods.py (2)
368-387: Add a test to assert deep-copy semantics for copy()Current tests only mock call flow. Add a test ensuring copied complex values don’t alias the source (mutating the source after copy doesn’t affect the target).
Example:
def test_copy_deepcopy(person: Person): # arrange: nested dict to observe aliasing person.pets[0].meta["traits"] = {"friendly": True} # act from fiftyone.server.utils.jsonpatch.methods import copy, get copy(person, "/pets/1/meta/traits", "/pets/0/meta/traits") # mutate source person.pets[0].meta["traits"]["friendly"] = False # assert copied value unchanged assert get(person, "/pets/1/meta/traits") == {"friendly": True}
256-276: Add tests for JSON Pointer escaping (~0 and ~1)Ensure get/remove work with keys containing “/” and “~”, per RFC 6901.
Example:
def test_pointer_escaping(person: Person): person.pets[0].meta["a/b"] = 1 person.pets[0].meta["a~b"] = 2 from fiftyone.server.utils.jsonpatch.methods import get, remove assert get(person, "/pets/0/meta/a~1b") == 1 assert get(person, "/pets/0/meta/a~0b") == 2 remove(person, "/pets/0/meta/a~1b") remove(person, "/pets/0/meta/a~0b") assert "a/b" not in person.pets[0].meta assert "a~b" not in person.pets[0].metafiftyone/server/utils/jsonpatch/methods.py (3)
52-76: Avoid brittle TypeError-string checks; explicitly handle lists and prefer mapping before attributeString-matching TypeError is fragile. Also, dict keys like "items" should not be shadowed by attributes. Handle lists by type, then mapping (getitem), then attribute.
Apply:
- for name in pointer.parts: - try: - value = getattr(value, name) - continue - except AttributeError as attr_err: - if hasattr(value, "__getitem__"): - try: - value = value[name] - continue - except TypeError as err: - if "list indices must be integers or slices" in str( - err - ): - idx = int(name) - - if not 0 <= idx < len(value): - raise IndexError( - "List index out of range" - ) from err - - value = value[idx] - continue - - raise attr_err + for name in pointer.parts: + # Lists: index explicitly + if isinstance(value, list): + try: + idx = int(name) + except ValueError as err: + raise AttributeError(f"List index must be int: {name}") from err + if not 0 <= idx < len(value): + raise AttributeError("List index out of range") + value = value[idx] + continue + # Mappings/other __getitem__ first to avoid attribute shadowing for dicts + if hasattr(value, "__getitem__"): + try: + value = value[name] # dict or custom mapping + continue + except Exception: + pass + # Attributes as fallback + try: + value = getattr(value, name) + continue + except AttributeError as attr_err: + raise attr_err
181-186: copy() should deep‑clone to match JSON value semanticsWithout deepcopy, copying nested dicts/lists aliases the source. Deep copy aligns with JSON Patch expectations.
Apply:
- value = get(src, from_pointer) + value = get(src, from_pointer) + # Deep copy to avoid aliasing mutable structures + try: + import copy as _copy + value = _copy.deepcopy(value) + except Exception: + passAlso add the import (top-level):
import copy as _copy # optional; used in copy()
32-33: Remove leftover stub do_it()Dead code; drop it.
Apply:
-def do_it(): ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/server/utils/jsonpatch/methods.py(1 hunks)tests/unittests/server/utils/jsonpatch/test_jsonpatch_methods.py(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/server/utils/jsonpatch/methods.py(1 hunks)
🔇 Additional comments (7)
fiftyone/server/utils/jsonpatch/methods.py (7)
18-29: LGTM!The function correctly converts paths to JSON pointers with appropriate error handling.
32-77: LGTM!The traversal logic correctly handles attributes, dict/list access, and list indices. The bounds check at line 65 is correct for read operations.
80-157: LGTM!The add operation correctly implements RFC 6902 semantics, including root replacement, object member addition/replacement, and array insertion. The bounds check at line 140 properly allows appending at the array end.
160-183: LGTM!The copy operation correctly implements RFC 6902 semantics by getting the value and adding it to the target location.
215-251: LGTM!The remove operation correctly implements RFC 6902 semantics. The existence check at line 232 properly uses
from_partsto preserve JSON Pointer token escaping.
254-270: LGTM!The replace operation correctly implements RFC 6902 semantics by removing and then adding the value.
273-305: LGTM!The test operation correctly validates equality at the target path, raising an appropriate error on mismatch.
4 participants
What changes are proposed in this pull request?
Adding endpoints to patch at the sample or field level following these guidelines: https://datatracker.ietf.org/doc/html/rfc6902
How is this patch tested? If it is not, please explain why.
Tested locally with unit tests and calling directly to the endpoint, then verifying to state of the sample in Mongo after edits are made.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Tests