feat: enforce lowercase opengraph property names BED-8071#2805
feat: enforce lowercase opengraph property names BED-8071#2805computator wants to merge 4 commits into
Conversation
computator
added
api
📝 WalkthroughWalkthroughThis PR adds property name validation constraints to node and edge ingest schemas. The propertyNames regex ChangesProperty name validation
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/api/src/services/upload/jsonschema/edge.json`:
- Around line 24-26: Confirm validation uses raw map keys (no normalization) by
noting streamdecoder.go calls schema.Validate(item) directly and that only
bomenc.NormalizeToUTF8 is applied; then add negative test cases asserting keys
with '.' and ':' are rejected by the edge schema (jsonschema/edge.json enforcing
propertyNames.pattern "^[a-z0-9_-]{1,128}$"), adding tests similar to the
existing uppercase/empty/space/length failures that expect schema validation to
fail and assert the error text contains the schema/propertyNames violation (or
the same error substring used by other negative cases).
🪄 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: 2fc12455-b8b4-434c-b6b7-158ebec22070
📒 Files selected for processing (3)
cmd/api/src/services/upload/jsonschema/edge.jsoncmd/api/src/services/upload/jsonschema/node.jsoncmd/api/src/services/upload/streamdecoder_test.go
urangel
left a comment
urangel
left a comment
There was a problem hiding this comment.
This looks to be failing the upload with a 400 as expected! 🙌
Since there have been a few updates that are not in the ticket, would you please add some notes on the changes so that we track the expectations accordingly? Mostly around the chosen pattern and the UI display related details.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/services/upload/streamdecoder_test.go (1)
1205-1272: ⚡ Quick winRemove redundant element type in composite literals (
gofmt -s).In each new case the slice element type is already declared, so repeating
propertyMatcher{...}inside[]propertyMatcher{...}is redundant.gofmt -s(and likely CI's format check) will flag/rewrite these on lines 1213, 1230, 1247, and 1264.♻️ Proposed simplification (apply to all four occurrences)
- End: &edgePiece{MatchBy: "property", PropertyMatchers: []propertyMatcher{ - propertyMatcher{Key: "TESTKey", Operator: "equals", Value: "asdf"}, - }}, + End: &edgePiece{MatchBy: "property", PropertyMatchers: []propertyMatcher{ + {Key: "TESTKey", Operator: "equals", Value: "asdf"}, + }},🤖 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/upload/streamdecoder_test.go` around lines 1205 - 1272, The composite literals in the test cases repeat the element type inside the slice (e.g., []propertyMatcher{ propertyMatcher{...} }) which is redundant; update each occurrence to use the short form by removing the inner type so the slices read []propertyMatcher{ {...} } (apply to the four cases constructing PropertyMatchers in the testPayload/Edges where propertyMatcher is used) and run gofmt -s to ensure formatting passes CI; search for propertyMatcher in this test to locate all spots.
🤖 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/services/upload/streamdecoder_test.go`:
- Around line 1205-1272: The composite literals in the test cases repeat the
element type inside the slice (e.g., []propertyMatcher{ propertyMatcher{...} })
which is redundant; update each occurrence to use the short form by removing the
inner type so the slices read []propertyMatcher{ {...} } (apply to the four
cases constructing PropertyMatchers in the testPayload/Edges where
propertyMatcher is used) and run gofmt -s to ensure formatting passes CI; search
for propertyMatcher in this test to locate all spots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: acde173a-d170-45d9-9dcd-6ff51260288f
📒 Files selected for processing (2)
cmd/api/src/services/upload/jsonschema/edge.jsoncmd/api/src/services/upload/streamdecoder_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/services/upload/jsonschema/edge.json
|
@urangel I added information about the character set and error discoverability to the PR description! |
2 participants
Description
property_matchers[].keyvalues with a regex patternCharacter Set
It is not realistic to implement this solely by checking for and rejecting uppercase characters, especially if enforcement is done via the schema files. Since enforcing lowercase will be a breaking change anyway, we decided to go with a character set that only allows lowercase Latin letters, numbers, dash, and underscore (
a-z0-9_-). We also set the maximum length as 128 characters.UI Error Discoverability
These changes use the same mechanism for reporting errors that all the other OpenGraph validation code uses. As such, the validation errors appear in the API logs and the HTTP response body. The UI however does not always surface these errors to the user. It did not seem logical to special-case this error to make it appear in the UI, and seemed more appropriate to create a separate ticket to surface all validation errors in the UI regardless of cause.
Motivation and Context
Resolves BED-8071
Not all systems that OpenGraph will interact with are case sensitive, so we are standardizing on lowercase property names.
How Has This Been Tested?
Uploaded some test data to verify it was rejected, and ran created tests locally.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests