fix(parametergroups): row-delete attribute, null guard, public class - #776
Conversation
…letes The row-delete op built by generate_parameter_group_patches emitted attribute "parameter", but the API filters row deletes on attribute == "parameters", so any parameter removal 400ed the whole PATCH.
The bulk ids endpoint pushes null for missing or inactive IDs, which crashed model validation. Filter nulls so not-found groups are omitted as documented.
The parameter groups API accepts and returns class "public"; add it to the shared SecurityClass enum so such groups validate.
The API returns associated documents under the Documents key; without the alias the field never populated.
|
Looks good - no bugs or correctness issues found. |
prasad-albert
left a comment
There was a problem hiding this comment.
LGTM! Verified backend contract parity, backward compatibility, caller ergonomics, and SDK conventions. Ready to merge.
# Conflicts: # tests/utils/test_patches.py
|
Looks good - no bugs or correctness issues found. |
|
Code Review Issues Found
Summary |
|
Looks good - no bugs or correctness issues found. |
# Conflicts: # tests/utils/test_patches.py
|
Looks good - no bugs or correctness issues found. |
CircleCI Integration Tests failedBranch: |
CircleCI Integration Tests failedBranch: |
|
Failure triage: (1) 8 errors + substance 504s = staging CAS service down (POST /api/v3/cas 504 Gateway Timeout across sheets/sds/product_design fixtures; substance metadata PATCH 504 is the same outage). (2) test_inventory_search_with_name_only_storage_location_filter = known partial-index poll flake, fixed by #798 predicate (pending merge). (3) test_task_property_calculation_evaluation = backend bug in api-datatemplate: datatemplate.service.js:783 reads uploadedFile.headers['x-amz-version-id'] unguarded, so a failed S3 upload surfaces as 500 'Cannot read properties of undefined'. Latent backend error-handling gap, not this diff (parameter_groups patch code only; failing call is a datatemplate create). Flagging the backend bug to the datatemplates owners. |
|
Code Review Issues Found
Summary |
CircleCI Integration Tests failedBranch: |
|
Looks good - no bugs or correctness issues found. |
What
Four parameter_groups parity fixes from the backend audit (§5.21), verified against
MoleculeEngineering/api-parametergroup:utils/_patch.py):generate_parameter_group_patchesemittedattribute: "parameter"for parameter row deletes, but the API filters row deletes onattribute === "parameters"(patchParameterGroup.jsviaconfig.jsPARAMETERS: 'parameters'), so any parameter removal 400ed the whole PATCH.get_by_idsnull guard (collections/parameter_groups.py): the bulk ids endpoint pushesnullentries for missing/inactive IDs (getBulkPRGByIds.jsbuildsItemsby indexing a found-map, so gaps serialize as null), which crashedParameterGroup(**None). Nulls are now skipped, matching the documented "groups not found are omitted" behavior.SecurityClass.PUBLIC(core/shared/enums.py): the API'sVALIDCLASSincludes"public"; added to the shared enum so groups with that class validate.documentsalias (resources/parameter_groups.py): the API returns associated documents underDocuments(parameterGroup.Documents = ...in the service); the field had no alias and never deserialized. Also fixed a mangled "See Also" fragment in its docstring.Why
SDK-166
How
_patch.py: single call-site change ("parameter"→"parameters"). The attribute name only feeds the row-delete datum, and the data-template caller of the same helper already passes"parameters", so no other path changes. Regression test added intests/utils/test_patches.py.get_by_ids: filtersNoneitems before model validation (one-line guard in the comprehension).SecurityClass: additive enum member, backwards compatible.documents:alias="Documents"; deserialization only, the field staysexclude=Trueon dump.PR #771 overlap awareness: this branch is off current main and touches none of #771's files (
exceptions.py,core/utils.py,core/pagination.py,collections/base.py,companies.py,workflows.py); the shared_patch.pyhunk is kept to the one call-site line in the row-delete path.Testing
uv run ruff format+uv run ruff check— clean (repo-wide).uv run pytest tests/utils tests/unit -q— 45 passed, including the newtest_parameter_group_row_delete_uses_parameters_attribute(verified it fails when the_patch.pyfix is reverted).uv run pytest tests/collections/test_parameter_groups.py --collect-only -q— 20 collected.SecurityClass("public")round-trip andDocumentsalias deserialization against payload shapes from the backend service.Cake session: https://agents.ai.albertinventdev.com/sessions/910a4725-9404-4336-81dd-e30bfef0ce73