Skip to content

fix(openapi): declare the removed device count the namespace carries - #7130

Merged
gustavosbarreto merged 1 commit into
masterfrom
fix/openapi-namespace-removed-device-count
Sep 17, 2026
Merged

gustavosbarreto merged 1 commit into
masterfrom
fix/openapi-namespace-removed-device-count

Conversation

@gustavosbarreto

Copy link
Copy Markdown
Member

What

The namespace schema now declares devices_removed_count, the fourth device counter the
server has been sending all along.

Why

models.Namespace.DevicesRemovedCount has serialized since cb2cf42, when removed
devices stopped weighing against the device limit. The schema gained the other three
counters and not this one. Nothing failed over it, because the schema sets no
additionalProperties: false and the validator only checks declared properties, so the
field travelled undeclared instead of loudly.

Split out of #7129, which found it while editing the same schema.

Changes

  • openapi/spec/components/schemas/namespace.yaml: the property, declared required
    to match its three siblings. The Go field has no omitempty, so every namespace
    response carries it.
  • ui/apps/console/src/tests/factories.ts and the admin drawer test: the generated
    TypeScript type now requires the field, so the two namespace fixtures set it. This is
    the whole cost of declaring it required, and it is why the change touches the console.

Testing

Worth confirming the generated client is what changed rather than any runtime behaviour.
npm run generate inside the ui container rewrites src/client/types.gen.ts, where
devices_removed_count: number becomes non-optional; npm run build -w @shellhub/console
typechecks against it. No server code changes.

@gustavosbarreto
gustavosbarreto requested review from a team as code owners September 17, 2026 02:10
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

Claude finished @gustavosbarreto's task in 6m 28s —— View job


Code Review Complete

Reviewed 3 files. 1 inline issue posted. Standards: 0 violations, 1 judgement call.
Spec: did not run. Correctness: 0.

Standards

No rule violations. The one judgement call is Duplicated Code: EditNamespaceDrawer.test.tsx:21-39 keeps a full Namespace literal that shadows the shared factory in tests/factories.ts:49, which is why this commit adds devices_removed_count: 0 in two places rather than one.

Correctness

No defects. The contract the schema now declares matches what the server sends: pkg/models/namespace.go:19 carries DevicesRemovedCount int64 with no omitempty and no pointer, so required cannot fail a response. All ten $refs to this schema were checked, including the one request body (admin@api@namespaces-update@{tenantID}.yaml:23), whose handler binds models.Namespace and whose console caller spreads the fetched namespace, so the new required field is always present. minimum: 0 is safe: the counters move in paired increment/decrement and are rebuilt by COUNT(*) FILTER. The two Namespace literals in ui/ are the two the diff updates, and the generated client is gitignored, so nothing was left to regenerate.

Spec

The Spec axis did not run. Neither the PR body nor commit c63f1ab references an issue — #7129 is the sibling pull request this was split out of, not a spec to review against.


To request another review round, comment /review.
· fix/openapi-namespace-removed-device-count

Comment thread ui/apps/console/src/pages/admin/namespaces/__tests__/EditNamespaceDrawer.test.tsx Outdated
@gustavosbarreto

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

Claude finished @gustavosbarreto's task in 5m 8s —— View job


Code Review Complete

Reviewed 3 files on all three axes: Standards (repo rules, language conventions, tests, code smells), Spec (does it implement the issue?) and Correctness (security and runtime defects). No new issues found. The code looks good as-is.

The Spec axis did not run: neither the PR body nor commit c63f1ab references an issue, and #7129 is the sibling pull request this was split out of rather than a spec to review against.

The one Standards judgement call this round surfaced — the full Namespace literal at ui/apps/console/src/pages/admin/namespaces/__tests__/EditNamespaceDrawer.test.tsx:21-39 shadowing the shared factory — was already posted inline in the previous round, so it is deduplicated rather than repeated.

What Correctness established this round, since the change makes a property required:

  • pkg/models/namespace.go:19 carries DevicesRemovedCount int64 with no omitempty and no pointer, and every handler answering this schema returns *models.Namespace, so required cannot fail a response.
  • Nine of the ten $refs are responses; the one request body (admin@api@namespaces-update@{tenantID}.yaml:23) already required eight other properties including the three sibling counters, and its only caller spreads the fetched namespace (editNamespaceSchema.ts:60-74). The counters also carry skipupdate at server/api/store/pg/entity/namespace.go:25-28, so that PUT cannot write them regardless.
  • The runtime validator checks responses only (server/api/pkg/openapi/openapi.go), so a newly-required field cannot turn a working request into a 400.
  • minimum: 0 holds: the increment and decrement paths are paired, and NamespaceSyncDeviceCounts rebuilds from COUNT(*) FILTER.
  • ui/ has exactly two full Namespace literals and both are in the diff; every other fixture routes through the factory with Partial overrides, so nothing else stops typechecking.
  • cloud/ references the field nowhere and defines no namespace schema of its own.

· fix/openapi-namespace-removed-device-count

@gustavosbarreto
gustavosbarreto force-pushed the fix/openapi-namespace-removed-device-count branch from c63f1ab to 15002c8 Compare September 17, 2026 02:43
@gustavosbarreto

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

Claude finished @gustavosbarreto's task in 5m 11s —— View job


Code Review Complete

Reviewed 3 files. 1 inline issue posted. Standards: 0 violations, 1 judgement call.
Spec: did not run. Correctness: 0.

Standards

The one judgement call is on the fixture collapse this round introduced: EditNamespaceDrawer.test.tsx:22-34 drops name from the override list while lines 103-105 and 352-354 still assert "my-namespace" (now sourced from factories.ts:51), and keeps members: [] and devices_accepted_count: 3, which no assertion and no code in EditNamespaceDrawer.tsx reads. The collapse itself is correct and does what the previous round asked.

The alias-and-shadow (import { mockNamespace as buildNamespace } followed by const mockNamespace = ...) is an outlier against the ~15 other console tests that use this factory unaliased, but it is a naming preference with no defect behind it, so it did not clear the bar.

Correctness

No defects. The factory's defaults match the deleted literal exactly for every field the conversion dropped — name, created_at, billing, devices_pending_count, devices_rejected_count — so no assertion changed meaning. settings is replaced wholesale by the override, so session_record: true still drives the pre-filled and toggled cases. buildEditNamespaceBody spreads the namespace, and the submit assertions use expect.objectContaining, so the new devices_removed_count riding along breaks nothing. No test does toEqual on a namespace, and no other Namespace literal remains in ui/. The schema YAML is well-formed with 11 unique required entries.

The suite could not be run here: ui/node_modules is absent and ./bin/docker-compose is unavailable in this environment. The above is from reading the files, not from a green run.

Spec

The Spec axis did not run. Neither the PR body nor commit 15002c8 references an issue — #7129 is the sibling pull request this was split out of, not a spec to review against.


To request another review round, comment /review.
· fix/openapi-namespace-removed-device-count

models.Namespace has serialized devices_removed_count since cb2cf42, when
removed devices stopped weighing against the device limit, and the schema never
gained the property. It sat alongside three siblings the schema does declare and
require, so the omission reads as an oversight rather than a decision.

Declared as required, matching the siblings: the field has no omitempty, so every
namespace response carries it. That makes the generated TypeScript type require
it too, which is why the namespace fixture now sets it.

The admin drawer test kept a full Namespace literal beside the shared factory,
which is what made this a two-place change. Collapsing it to a factory call with
overrides puts the counters in one place, so the next one added to the schema
does not reach into that file at all.
@gustavosbarreto
gustavosbarreto force-pushed the fix/openapi-namespace-removed-device-count branch from 15002c8 to 6355ebd Compare September 17, 2026 02:58
@gustavosbarreto

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

Claude finished @gustavosbarreto's task in 5m 3s —— View job


Code Review Complete

Reviewed 3 files on all three axes: Standards (repo rules, language conventions, tests, code smells), Spec (does it implement the issue?) and Correctness (security and runtime defects). No new issues found. The code looks good as-is.

The Spec axis did not run because there is no issue to review against: neither the PR body nor commit 6355ebd carries an issue reference, and #7129 is the sibling pull request this was split out of rather than a spec.

The only Standards candidate this round was the aliased import (mockNamespace as buildNamespace shadowed by the local const mockNamespace). It is dropped rather than posted: the previous round's summary already stated it, and on a second look the alias earns itself — the other ~15 console tests call the factory inline per case, while this file needs one shared fixture referenced eight times, so the local name is doing real work. A naming preference with no defect behind it does not clear the bar.

Both fixes from the previous rounds check out at HEAD. name: "my-namespace" is back in the overrides at line 23, so the assertions at lines 102-104 and 351-353 own the value they read instead of inheriting it from factories.ts:51; members and devices_accepted_count are gone, and the five surviving overrides map one-to-one onto what the assertions read (name → 102, 351; owner → 202; tenant_id → 179, 203; max_devices → 109, 182; settings.session_record → 114, 183). The three other settings keys are not decoration — namespaceSettings.yaml:36-40 marks all four required, so a partial override would not typecheck.

What Correctness established, since the change makes a response property required:

  • The validator is response-only. server/api/pkg/openapi/openapi.go:118-164 calls ValidateResponse and nothing calls ValidateRequest anywhere in server/ or pkg/, so a newly-required field cannot turn a working request into a 400.
  • The server cannot omit the field. pkg/models/namespace.go:19 is a value-type int64 with no omitempty and no custom marshaller, and all nine response $refs resolve to handlers in server/api/routes/nsadm.go serializing models.Namespace directly. The tenth $ref is the admin update request body, bound without a schema check, and its only caller spreads the fetched namespace.
  • minimum: 0 holds. Every decrement is paired with a row transition, the column is bigint DEFAULT 0 NOT NULL, the entity carries skipupdate so a client-supplied value cannot be written, and NamespaceSyncDeviceCounts rebuilds from COUNT(*) FILTER.
  • The description is accurate. NamespaceDeviceLimit.IsReached compares only DevicesAcceptedCount >= MaxDevices, and that is the sole limit gate, so a removed device genuinely does not weigh against max_devices.
  • The devices_accepted_count 3 → 0 drift from the fixture collapse is inert. EditNamespaceDrawer.tsx reads only name, tenant_id, max_devices and settings.*, and no assertion in the file reads a count.
  • cloud/ defines no namespace schema of its own and references the field nowhere.

The suite was not run here: ui/node_modules is absent and ./bin/docker-compose is unavailable in this environment. The above is from reading the source, not from a green run.


· fix/openapi-namespace-removed-device-count

@gustavosbarreto
gustavosbarreto merged commit efc22bf into master Sep 17, 2026
47 checks passed
@gustavosbarreto
gustavosbarreto deleted the fix/openapi-namespace-removed-device-count branch September 17, 2026 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant