Update JSON schema for new_qname #74
Conversation
WalkthroughThe pull request updates the JSON schema defined in Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
evrec/schema/new_qname.json (3)
10-10: Consider constraining additional properties.Setting
additionalPropertiestotruewithout any constraints could lead to schema drift and make it harder to maintain compatibility. Consider either:
- Setting it to
falseto be strict- Using
patternPropertiesto enforce naming conventions
19-22: Specify UUID version for message_id.While using UUID format is good for uniqueness, consider specifying which UUID version should be used (e.g., UUIDv4 for randomness or UUIDv5 for deterministic generation).
45-48: Add minimum constraint to flags field.For consistency with other numeric fields, consider adding a minimum constraint to the flags field.
"flags": { "description": "Flag Field (QR/Opcode/AA/TC/RD/TA/Z/RCODE)", - "type": "integer" + "type": "integer", + "minimum": 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
evrec/schema/new_qname.json(1 hunks)
🔇 Additional comments (2)
evrec/schema/new_qname.json (2)
2-3: LGTM! Good improvements to schema metadata.The changes improve the schema by:
- Using the latest stable JSON Schema draft (2020-12)
- Adding a proper schema identifier with versioning
5-9: Verify the impact of making timestamp optional.The removal of
timestampfrom required properties is a potentially breaking change. Ensure that all consumers of this schema are prepared to handle records without timestamps.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for code that might assume timestamp is present rg -l "timestamp.*new_qname" | xargs rg "timestamp"Length of output: 338
Timestamp Optional Impact Confirmed
- The search reveals that while the JSON schema still defines a "timestamp" property, its removal from the required list aligns with existing handling in the codebase. In particular,
evrec/__init__.pyboth imports a build-generated__timestamp__and then explicitly sets it toNone, which indicates that downstream code already anticipates aNoneor missing value.- The Makefile continues to generate a timestamp, so when present, it will be used; however, the default behavior when absent appears to be intentional.
- As an additional suggestion, consider adding validation constraints (e.g., using
"additionalProperties": false) to the schema to mitigate potential schema drift.
| "qname": { | ||
| "description": "Query Name", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Restore domain name validation for qname.
The change from $ref: "#/$defs/domain_name" to a simple string type removes domain name validation. This could allow invalid domain names to pass schema validation.
Consider either:
- Restoring the reference to domain_name definition
- Adding a pattern constraint for valid domain names:
"qname": {
"description": "Query Name",
"type": "string"
+ "pattern": "^([a-zA-Z0-9-]+\\.)*[a-zA-Z0-9-]+\\.[a-zA-Z]{2,}$"
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Refactor