feat: short-name resolution via derivation + schema $id index #1783 - #1813
feat: short-name resolution via derivation + schema $id index #1783#1813KirylKurnosenka wants to merge 8 commits into
Conversation
Makes every materialized platform-bucket config entity short-name addressed, inbound and outbound, via deterministic derivation (no per-entity flag, no stored alias index) for models/interceptors/ roles/applications/toolsets, and adds a $id -> canonical-id index so blob-stored app-type/catalog schemas resolve by their JSON-Schema $id.
This comment has been minimized.
This comment has been minimized.
APP_TYPE_SCHEMA's urlSegment is "schemas", not "application_type_schemas" (its group and urlSegment differ, unlike CATALOG_SCHEMA), so the test's "canonical id" example didn't match what MergedConfigStore actually generates.
This comment has been minimized.
This comment has been minimized.
Code-review follow-up on the short-name resolution feature: - Deployment-id uniqueness now dedupes on the derived short name (not the raw map key), and is enforced on the partial-update and /v1/admin/apply paths too, not just full rebuilds. - RateLimiter/ConsentService fall back to a canonical-id match so pre-existing role limits and consent records keep resolving now that deployment names revert to short form. - AnalyticsLogContext normalizes canonical-id interceptor refs before comparing against the (always short-name) execution path. - Added a bucket-scope guard to MergedConfigStore's rebuild shadow step, mirroring the existing replica-event guard. - Deduplicated the several lastSegment implementations into PlatformCanonicalIdUtil. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…1783 recordSchemaAlias never evicted a schema's previous $id alias when its $id changed, leaving a dangling entry pointing at a body that no longer carries it. Fix by evicting any alias currently held by this canonical id before recording the new one (same removeIf-by-value pattern already used on delete), and add a write-time check rejecting a write whose $id is already claimed by a different canonical id (wired into the single- entity PUT, admin-apply real-apply, and admin-apply precheck paths). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| * in {@link #applicationTypeSchemas}, since a schema's $id is not derivable from its path. | ||
| */ | ||
| @JsonIgnore | ||
| private Map<String, String> schemaAliasesById = Map.of(); |
There was a problem hiding this comment.
applicationSchemaAliasesById
There was a problem hiding this comment.
Renamed to applicationSchemaAliasesById (field/getter/setter, and all call sites).
| * migrated blob entry. A schema's $id is not derivable from its path, so unlike {@link | ||
| * #resolve}, the alias index must be maintained explicitly (see {@code MergedConfigStore}). | ||
| */ | ||
| private static String resolveSchema(Map<String, String> schemas, Map<String, String> aliasesById, URI schemaId) { |
There was a problem hiding this comment.
javax.annotation.Nullable isn't on the config module's classpath (only pulled in transitively by server), and adding a direct dependency just for this annotation felt disproportionate. Documented nullability via @return ... or {@code null} if ... javadoc on getCustomApplicationSchema/getCatalogSchema/resolveSchema instead.
| private List<String> globalInterceptors = List.of(); | ||
|
|
||
| /** | ||
| * $id → canonical-id index for {@code platform}-bucket schema entities, built at rebuild |
There was a problem hiding this comment.
The javadoc is not clear what key is and what value is.
There was a problem hiding this comment.
Reworded to explicitly state key = schema $id, value = canonical id, and added a concrete example.
| ShareResourceLimit limit = null; | ||
| for (String userRole : userRoles) { | ||
| ShareResourceLimit candidate = Optional.ofNullable(roles.get(userRole)).map(Role::getShare).map(limits -> limits.get(resourceType.name())).orElse(null); | ||
| ShareResourceLimit candidate = Optional.ofNullable(config.getRole(userRole)).map(Role::getShare).map(limits -> limits.get(resourceType.name())).orElse(null); |
There was a problem hiding this comment.
why do we need these changes?
There was a problem hiding this comment.
Blob-stored Role entities are now short-name addressed via canonical id (e.g. roles/platform/<name>), same as models/applications/toolsets/interceptors in this PR. A raw roles.get(userRole) map lookup would miss a role migrated to the blob store, since its map key is the canonical id, not the short name. Config.getRole(id) goes through the new resolve() helper (short-name-first, canonical-id fallback), so this swap is part of the same call-site sweep as the other accessors — not unrelated churn.
| } | ||
|
|
||
| /** | ||
| * Records the {@code $id → canonicalId} alias read from a schema's body, and removes the |
There was a problem hiding this comment.
Who will read a such long javadoc?
The method has 5 lines the doc is more than 10 long lines?
There was a problem hiding this comment.
Trimmed both this method's javadoc and removeEntityInPlace's down to their essential contract.
rejectSchemaIdCollision silently treated any non-APP_TYPE_SCHEMA type as CATALOG_SCHEMA via a fallback ternary. Switch to an explicit type check that throws for anything other than the two supported schema types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
recordSchemaAlias evicted a stale alias via aliasesById.values().removeIf, an O(index size) scan on every schema write/delete (and, since it was always a guaranteed no-op mid-rebuild, effectively O(n^2) extra work across a full rebuild for no benefit). Map.put/remove already return the previous value at a key, so callers now pass that previous body through and recordSchemaAlias reads its $id directly and evicts by that key - O(1) instead of a full index scan for every schema mutation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rename schemaAliasesById to applicationSchemaAliasesById to disambiguate from catalogSchemaAliasesById, clarify its key/value semantics with an example, document nullability on schema accessors, and trim the recordSchemaAlias/removeEntityInPlace javadocs to their essential contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Makes every materialized
platform-bucket config entity short-name addressed, inbound and outbound, via deterministic derivation (no per-entity flag, no stored alias index), and adds a$id → canonical-idindex so blob-stored app-type/catalog schemas resolve by their JSON-Schema$id.Applicable issues
Description of changes
Config.java: addedresolve(...)derivation helper; rewroteselectDeploymentto use it for applications/models/toolsets/interceptors; addedgetModel/getRole/getInterceptoraccessors; addedschemaAliasesById/catalogSchemaAliasesByIdwith$id-fallback ingetCustomApplicationSchema/getCatalogSchema(refactored into a sharedresolveSchemahelper).ConfigPostProcessor.java:entity.setName(lastSegment(...))everywhere instead of the raw map key; resolve-aware fix forvalidateCrossReferences(a model referencing a migrated interceptor by short name is no longer wrongly treated as dangling); addedvalidateSingleApplication/validateSingleToolSet.MergedConfigStore.java: blob-shadows-file removal for models/interceptors/roles/applications/toolsets (gated on successful decryption during full rebuild; immediate on partial-update writes);$id-alias index maintenance for schemas, including shadowing the file entry keyed by the same$idso migrated schemas don't appear twice in$id-keyed listings; fixed ashallowClonegap that would have dropped the schema alias index on every partial update; removed the now-unnecessaryAddedEntitywrapper record.ModelController,DeploymentController,RateLimiter,ShareService,CollectResponseAttachmentsFn,ResourceController,BlobEntityValidator,BaseInterceptorControllernow resolve through the new accessors instead of raw mapget/containsKey.ConfigTestwith derivation/accessor/alias coverage; updatedConfigPostProcessorTest,CanonicalIdListingTest,MergedConfigStoreApiTestfor short-name outbound behavior; added shadow-removal integration tests for models/interceptors/roles/applications/toolsets/schemas.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.