refactor(instances): prefix-collision guard, typed lint rule, API symmetry (2/2) - #79
Open
dianaKhortiuk-frontegg wants to merge 3 commits into
Open
refactor(instances): prefix-collision guard, typed lint rule, API symmetry (2/2)#79dianaKhortiuk-frontegg wants to merge 3 commits into
dianaKhortiuk-frontegg wants to merge 3 commits into
Conversation
Review feedback on #75. - InstanceRegistry now rejects two instances that resolve to the same schemaPrefix, whether from colliding explicit overrides or from two vendorIds that normalise to the same value ("ACME-CORP" and "acme_corp" both derive v_acme_corp). Without this two vendors silently share one SpiceDB namespace, which is the leak this work exists to prevent. Two explicitly-legacy instances are rejected on the same grounds. - readSchemaFor splits the schema by tracking brace depth rather than by a regex lookahead, so blocks that are indented or preceded by comments are attributed correctly instead of being silently dropped or merged. - UnknownInstanceException and InstanceIdRequiredException no longer put the configured instance list in the message; it stays available as configuredInstanceIds for debugging, so a caller that surfaces err.message cannot leak the vendor list. - Dropped the InstanceResolutionException branch from the query catch blocks; resolution happens in the caller, so it was unreachable. - Dropped the redundant defaultInstanceId argument at both InstanceRegistry call sites now that the parameter defaults from the configuration object. FR-26219
…rule The guard was regex over source text: it only recognised three call shapes, so a request built any other way was invisible to it with no failure signal. That was the one thing holding the scoping invariant, so it is now a real ESLint rule with type information. eslint-rules/require-scoped-object-type.js flags any objectType / resourceObjectType / subjectObjectType / resourceType property whose contextual type originates in @authzed/authzed-node and whose value is not a scope.type(...) call. Keying on the declaring package is what separates a genuine request field from a log payload or a value read back off a response, which the regex version could only approximate. Contextual types arrive as PartialMessage<T> | undefined, so the rule walks union members and generic arguments rather than reading the property off the top-level type, which resolves to nothing. Verified against three planted violations: a raw type inside v1.X.create(), a plain object literal passed straight to client.checkPermission() (the shape the regex guard missed entirely), and a RelationshipFilter.resourceType. All three are reported; the existing log payloads and response reads are not. schema-scope.guard.spec.ts is deleted. Its replacement asserts the rule stays wired as an error, with a files glob covering src and with parserOptions .project set — without type information the rule silently matches nothing, which is the failure mode worth guarding. Addresses review finding 2 on #75.
…method lookupTargetEntities, lookupEntities and lookupEntitlements took instanceId inside the request object while isEntitledTo and isEntitledToMany took it as a trailing options argument. All five now use the options argument, so callers have one thing to remember. Doing it now because the multi-instance API is unreleased; once shipped this becomes a breaking change. Addresses review finding 7 on #75.
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.
Part 2 of 2, split out of #75. Stacked on #78 — merge that first. Checks stay pending until #78 merges and GitHub retargets this to master; verified locally in the meantime: 252 tests, 0 lint errors,
tscclean.src/instances/instance-registry.ts— rejects two instances resolving to the same prefix: colliding explicit overrides, two vendorIds that normalise to the same value (ACME-CORPandacme_corpboth givev_acme_corp), and two explicitly-legacy instances. Without it two vendors silently share one namespace.src/instances/schema-prefix.ts— prefix is now a required argument; the runtime guard and a silent''default no longer disagree.eslint-rules/require-scoped-object-type.js— new. Type-aware rule replacing the regex guard: flags any object-type field whose contextual type comes from@authzed/authzed-nodeand isn't built viascope.type(). Wired ineslint.config.jsoversrc/**withparserOptions.project.src/instances/schema-scope.guard.spec.ts— deleted, replaced byschema-scope.lint-rule.spec.tsasserting the rule stays wired.src/spicedb/spicedb-entitlements.client.ts—readSchemaForsplits by brace depth instead of a regex lookahead; exception messages no longer carry the configured instance list;instanceIdis a trailing options argument on all five public methods, matchingisEntitledTo.