feat: Add KindInfo to Extension Upsert - BED-8764#2986
Conversation
- Resolved conflicts in cmd/api/src/model/graphschema.go - Kept entity panel validation errors (ErrInvalidKindInfoKey, etc.) - Kept KindInfoInputs type definition - Resolved conflicts in cmd/api/src/database/graphschema_integration_test.go - Kept TestDatabase_UpdateKindInfo test function Conflicts arose from merging main which included BED-8858 PR #2962
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOpenGraph extensions now accept per-kind information panels with titled, positioned markdown. The API converts and validates these panels, database upserts reconcile panel records, and OpenAPI schemas and examples document the payload. ChangesKind info support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OpenGraphClient
participant OpenGraphSchemaIngest
participant GraphExtensionInput
participant UpsertOpenGraphExtension
participant KindInfoRepository
OpenGraphClient->>OpenGraphSchemaIngest: submit info panels
OpenGraphSchemaIngest->>GraphExtensionInput: convert and validate KindInfoInputs
GraphExtensionInput->>UpsertOpenGraphExtension: provide validated extension input
UpsertOpenGraphExtension->>KindInfoRepository: reconcile create, update, and delete records
KindInfoRepository-->>UpsertOpenGraphExtension: return persistence result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
400-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a position-swap update case. The update fixtures keep positions distinct (
overview:1,examples:3), so they don't exercise reordering existing infos. A case that swaps two positions within one upsert would cover the reconcile/unique-constraint concern flagged incmd/api/src/database/upsert_schema_extension.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/api/src/database/upsert_schema_extension_integration_test.go` around lines 400 - 428, The update fixtures in the test setup should exercise reordering existing infos by swapping the positions of at least two retained entries within the same upsert. Adjust the relevant KindInfoInputs in the update case around the existing overview/examples reconciliation so their positions exchange while preserving the expected add, update, and delete assertions.cmd/api/src/services/opengraphschema/extension.go (1)
39-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the two error-translation checks.
Both branches perform the same wrap (
fmt.Errorf("%w: %w", model.ErrGraphExtensionValidation, err)); a single slice of translatable sentinel errors checked witherrors.Isin a loop (orerrors.Join-based matcher) would reduce duplication and make it easier to add future kind-info error translations.♻️ Suggested refactor
- if model.ErrIsGraphSchemaDuplicateError(err) { - return schemaExists, fmt.Errorf("%w: %w", model.ErrGraphExtensionValidation, err) - } - // Translate kind info database errors to validation errors - if errors.Is(err, model.ErrKindInfoKindNotFound) || - errors.Is(err, model.ErrKindInfoDuplicatePosition) || - errors.Is(err, model.ErrKindInfoDuplicateInfoKey) { - return schemaExists, fmt.Errorf("%w: %w", model.ErrGraphExtensionValidation, err) - } + if model.ErrIsGraphSchemaDuplicateError(err) || isKindInfoValidationError(err) { + return schemaExists, fmt.Errorf("%w: %w", model.ErrGraphExtensionValidation, err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/api/src/services/opengraphschema/extension.go` around lines 39 - 48, Consolidate the duplicate error translation branches in the surrounding error-handling function by defining one collection or matcher for the graph-schema duplicate error and the listed kind-info errors, then perform a single check before applying the existing validation-error wrap. Preserve the current matching behavior and return values while making future translatable errors easy to add.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/go/openapi/src/schemas/model.graph-extension.kind-info-definition.yaml`:
- Around line 17-26: Align the additionalProperties schema with its description
by updating the required list for each panel object to include markdown
alongside title and position, ensuring every KindInfo panel requires markdown
content.
---
Nitpick comments:
In `@cmd/api/src/database/upsert_schema_extension_integration_test.go`:
- Around line 400-428: The update fixtures in the test setup should exercise
reordering existing infos by swapping the positions of at least two retained
entries within the same upsert. Adjust the relevant KindInfoInputs in the update
case around the existing overview/examples reconciliation so their positions
exchange while preserving the expected add, update, and delete assertions.
In `@cmd/api/src/services/opengraphschema/extension.go`:
- Around line 39-48: Consolidate the duplicate error translation branches in the
surrounding error-handling function by defining one collection or matcher for
the graph-schema duplicate error and the listed kind-info errors, then perform a
single check before applying the existing validation-error wrap. Preserve the
current matching behavior and return values while making future translatable
errors easy to add.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f3795cd8-4331-45df-8046-0379e4d17ab6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
cmd/api/src/api/v2/opengraphschema.gocmd/api/src/api/v2/opengraphschema_internal_test.gocmd/api/src/api/v2/opengraphschema_test.gocmd/api/src/database/customnode_integration_test.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/model/graphschema_test.gocmd/api/src/services/opengraphschema/extension.gogo.modpackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/paths/opengraph.extensions.yamlpackages/go/openapi/src/schemas/model.graph-extension.kind-info-definition.yamlpackages/go/openapi/src/schemas/model.graph-extension.payload.yamlschemas/open_graph/dog-park-schema.json
| description: | | ||
| A map of named info panels used in extension definitions. | ||
| Each property key is a stable panel identifier, and each property value | ||
| must be a valid KindInfo object with title, position, and content. | ||
| propertyNames: | ||
| pattern: '^[a-z0-9_-]{1,128}$' | ||
| description: "Keys may only include lowercase alphanumeric characters, hyphens, and underscores" | ||
| additionalProperties: | ||
| type: object | ||
| required: [title, position] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Schema description and required list are inconsistent.
The description states each panel "must be a valid KindInfo object with title, position, and content," but required only includes [title, position]. The markdown object (which contains content) is not required. If every info panel is expected to have markdown content, add markdown to the required list. If markdown is intentionally optional, update the description to reflect that.
🔧 Proposed fix: add `markdown` to required list
additionalProperties:
type: object
- required: [title, position]
+ required: [title, position, markdown]
properties:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: | | |
| A map of named info panels used in extension definitions. | |
| Each property key is a stable panel identifier, and each property value | |
| must be a valid KindInfo object with title, position, and content. | |
| propertyNames: | |
| pattern: '^[a-z0-9_-]{1,128}$' | |
| description: "Keys may only include lowercase alphanumeric characters, hyphens, and underscores" | |
| additionalProperties: | |
| type: object | |
| required: [title, position] | |
| description: | | |
| A map of named info panels used in extension definitions. | |
| Each property key is a stable panel identifier, and each property value | |
| must be a valid KindInfo object with title, position, and content. | |
| propertyNames: | |
| pattern: '^[a-z0-9_-]{1,128}$' | |
| description: "Keys may only include lowercase alphanumeric characters, hyphens, and underscores" | |
| additionalProperties: | |
| type: object | |
| required: [title, position, markdown] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/go/openapi/src/schemas/model.graph-extension.kind-info-definition.yaml`
around lines 17 - 26, Align the additionalProperties schema with its description
by updating the required list for each panel object to include markdown
alongside title and position, ensuring every KindInfo panel requires markdown
content.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/model/graphschema.go (1)
143-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd duplicate InfoKey and Position validation in
validateKindInfoThe function doesn't check for duplicate
InfoKeyorPositionvalues within the same kind'sInfolist. The database catches these via uniqueness constraints (confirmed by integration testserror_-_update_to_duplicate_info_keyanderror_-_update_to_duplicate_position), but model-layer validation would provide clearer error messages and avoid unnecessary database round-trips.This requires adding two new exported error variables (e.g.,
ErrDuplicateKindInfoKey,ErrDuplicateKindInfoPosition).♻️ Proposed refactor: add duplicate detection
func validateKindInfo(kindName string, info KindInfoInputs) error { + var ( + seenKeys = make(map[string]struct{}) + seenPositions = make(map[int32]struct{}) + ) + if len(info) > 100 { return fmt.Errorf("%w for kind %s: has %d entries", ErrTooManyKindInfoEntries, kindName, len(info)) } for _, infoEntry := range info { if !kindInfoKeyPattern.MatchString(infoEntry.InfoKey) { return fmt.Errorf("%w: key '%s' in kind %s", ErrInvalidKindInfoKey, infoEntry.InfoKey, kindName) } else if infoEntry.Position < 1 { return fmt.Errorf("%w: position %d for info key '%s' in kind %s", ErrInvalidKindInfoPosition, infoEntry.Position, infoEntry.InfoKey, kindName) } else if err := validateKindInfoContent(infoEntry.Content); err != nil { return fmt.Errorf("info key '%s' in kind %s: %w", infoEntry.InfoKey, kindName, err) + } else if _, exists := seenKeys[infoEntry.InfoKey]; exists { + return fmt.Errorf("%w: duplicate key '%s' in kind %s", ErrDuplicateKindInfoKey, infoEntry.InfoKey, kindName) + } else if _, exists := seenPositions[infoEntry.Position]; exists { + return fmt.Errorf("%w: duplicate position %d in kind %s", ErrDuplicateKindInfoPosition, infoEntry.Position, kindName) } + + seenKeys[infoEntry.InfoKey] = struct{}{} + seenPositions[infoEntry.Position] = struct{}{} } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/api/src/model/graphschema.go` around lines 143 - 158, Update validateKindInfo to track previously seen InfoKey and Position values while iterating through info, returning dedicated exported errors such as ErrDuplicateKindInfoKey and ErrDuplicateKindInfoPosition when either value repeats within the same kind. Preserve the existing validation order and error context, and define the new error variables alongside the other model validation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/api/src/model/graphschema.go`:
- Around line 143-158: Update validateKindInfo to track previously seen InfoKey
and Position values while iterating through info, returning dedicated exported
errors such as ErrDuplicateKindInfoKey and ErrDuplicateKindInfoPosition when
either value repeats within the same kind. Preserve the existing validation
order and error context, and define the new error variables alongside the other
model validation errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f076f1b7-cfba-40b1-a769-de46e0e2c164
📒 Files selected for processing (5)
cmd/api/src/api/v2/opengraphschema.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/model/graphschema_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/api/v2/opengraphschema.go
| Info: infoInputs, | ||
| }) | ||
| } | ||
| /* |
There was a problem hiding this comment.
thanks for cleaning this up!
There was a problem hiding this comment.
I think im going to cut another ticket to get all the old property stuff removed, there's an unused table as well as db functions, but its a little much to include here.
|
|
||
| result = append(result, model.KindInfoInput{ | ||
| InfoKey: key, | ||
| Title: payload.Title, |
There was a problem hiding this comment.
Should we include validation on Title key? Currently the API is returning 500 status on missing the key or passing an empty string.
There was a problem hiding this comment.
Yeah I think we should either return a 400 with a descriptive error message if we want to have non-empty titles or allow empty titles and avoid the 500. Probably the former
There was a problem hiding this comment.
Whoops, thought i had a validation check for title, definitely need that and I agree with @urangel that we should be returning a 400.
Will get this added in a min
| } | ||
| } | ||
| ], | ||
| "properties": [], |
There was a problem hiding this comment.
I know this isn't apart of your PR but properties in the upsert extension schema is dead now, right?
There was a problem hiding this comment.
Yes! Thank you for reminding me, i add a commit to remove this in a sec
|
need a corresponding enterprise branch but looks good! excited to see the entity panel work for OG!! |
| Position: math.MaxInt32 + 1, // would silently wrap when cast to int32 | ||
| Markdown: []byte(`{"content":"# Test"}`), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Might be worth a test case for zero position too - I don't think it would throw an error as it is now, but it probably should?
AD7ZJ
left a comment
AD7ZJ
left a comment
There was a problem hiding this comment.
One nit on the position validation, otherwise looks good to me!
6 participants
Description
Motivation and Context
Resolves: BED-8764
As an OpenGraph Extension Author, I want the /api/v2/extensions API to support KindInfo entries for node and relationship kinds so that I can keep those definitions in sync with my extension.
How Has This Been Tested?
Types of changes
Checklist:
Summary by CodeRabbit
Summary