feat: Add updatedAt support for orderBy and projection in query specs - #227
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Specification (spec) client models to support UPDATED_AT in QuerySpecificationsRequest for both order_by and projection, aligning the Python client with the server-side “query-specs” API capabilities.
Changes:
- Added
UPDATED_ATtoSpecificationOrderByandSpecificationProjectionenums used byQuerySpecificationsRequest. - Exported
SpecificationOrderByfromnisystemlink.clients.spec.modelsfor public import. - Added integration tests covering ordering by
updatedAt(ascending/descending) and projecting onlyupdatedAt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/integration/spec/test_spec.py |
Adds integration coverage for UPDATED_AT ordering and projection behavior. |
nisystemlink/clients/spec/models/_query_specs.py |
Extends query-spec enums to include UPDATED_AT for order-by and projection. |
nisystemlink/clients/spec/models/__init__.py |
Re-exports SpecificationOrderBy from the models package. |
Suppressed comments (1)
tests/integration/spec/test_spec.py:440
- Same issue as the ascending-order test: filtering out
Noneupdated_atvalues can make this pass even whenupdated_atisn’t returned consistently. Assert the field exists for every spec before validating the descending sort order.
updated_ats = [spec.updated_at for spec in response.specs if spec.updated_at]
assert updated_ats == sorted(updated_ats, reverse=True)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
tests/integration/spec/test_spec.py:333
- The projection assertion aggregates non-None keys across all returned specs (and uses
vars(spec)), so it can pass even if only a subset of specs includeupdated_at. To make this test reliably validate projection behavior, assert per-spec usingmodel_dump(exclude_unset=True)and ensure each spec includes exactly the projected fields (and thatupdated_atis present).
response = client.query_specs(request)
specs = [vars(spec) for spec in response.specs or []]
spec_columns = {
key for spec in specs for key in spec.keys() if spec[key] is not None
}
tests/integration/spec/test_spec.py:495
- This test doesn't assert that the update call succeeded. If
update_specspartially fails, the ordering assertions may become flaky or misleading. Capture and assert the update response (consistent with earlier tests in this file) before querying and asserting sort order.
client.update_specs(
UpdateSpecificationsRequest(
specs=[
UpdateSpecificationsRequestObject(
id=spec_1.id,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
tests/integration/spec/test_spec.py:468
- This test assumes the queried specs are always at positions 0 and 1. If additional specs exist for the same product (e.g., due to test ordering or eventual cleanup), the test could fail or validate the wrong items. Make the assertion relative by checking the index ordering of the two created spec IDs within the returned list.
This issue also appears on line 519 of the same file.
assert response.specs
# spec_2 was not updated, so it has an earlier updated_at — must come first
assert response.specs[0].spec_id == spec_2_id
assert response.specs[1].spec_id == spec_1_id
tests/integration/spec/test_spec.py:522
- This test assumes the queried specs are always at positions 0 and 1. If additional specs exist for the same product (e.g., due to test ordering or eventual cleanup), the test could fail or validate the wrong items. Make the assertion relative by checking the index ordering of the two created spec IDs within the returned list.
assert response.specs
# spec_1 was updated last, so it has a later updated_at — must come first
assert response.specs[0].spec_id == spec_1_id
assert response.specs[1].spec_id == spec_2_id
tests/integration/spec/test_spec.py:333
- The projection assertion is brittle: it uses
vars(spec)and unions non-None keys across all specs, so the test can still pass even if only a subset of returned specs includesupdated_at(or if Pydantic internals appear invars). Prefermodel_dump(exclude_unset=True)and assert per-spec that only the projected fields are present.
response = client.query_specs(request)
specs = [vars(spec) for spec in response.specs or []]
spec_columns = {
key for spec in specs for key in spec.keys() if spec[key] is not None
}
Summary
Adds
UPDATED_ATas a supported value fororder_byandprojectioninQuerySpecificationsRequest, consistent with the server-side changes made in the Skyline repo.Pull Request 1293910: Specification Management | Add updatedAt field to order by options in /query-specs
Technical Debt 4025900: Specs Service Python client | Add UpdatedAt support for query specs
Changes
SpecificationOrderBy: AddedUPDATED_ATenum value, allowing specs to be sorted by last modification time.SpecificationProjection: AddedUPDATED_ATenum value, allowingupdatedAtto be requested as a projected field (consistent withCREATED_AT).models/__init__.py: ExportedSpecificationOrderByso it is importable fromnisystemlink.clients.spec.models.Tests
Added integration tests in
tests/integration/spec/test_spec.py:updatedAtordering — creates two specs, updates one, and verifies the unmodified spec (olderupdated_at) comes first.updatedAtordering — same setup, verifies the updated spec (newerupdated_at) comes first.UPDATED_ATprojection — addedUPDATED_ATto the existingtest__query_spec_projection_columns__columns_returnedtest alongsideSPEC_IDandNAME, verifying all three fields are returned.