Implements conditional subschemas based on component type - #774
Implements conditional subschemas based on component type#774stevespringett wants to merge 1 commit into
Conversation
Signed-off-by: Steve Springett <steve@springett.us>
There was a problem hiding this comment.
Pull request overview
This PR implements conditional subschemas for component properties based on component type, as specified in issue #638. The changes enforce that certain properties (modelCard, data, cryptoProperties) can only be used with specific component types.
Changes:
- Removed
modelCard,data, andcryptoPropertiesfrom the main component properties section - Added conditional subschemas using
allOfwithif-then-elselogic to enforce type-specific property usage - Added a new conditional requirement for hardware components to prevent
swidandpurlspecification
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @stevespringett — quick catch on “Version Range Requirement” (around line 281), unless I’m misunderstanding the intent: Right now the rule triggers on the presence of If the intent is “ {
"if": { "required": ["versionRange"] },
"then": {
"properties": { "isExternal": { "const": true } },
"required": ["isExternal"]
}
}One small wording consistency nit: the Version Range rule description currently reads “versionRange must not be present when isExternal is false”, while the |
|
@Mehrn0ush thank you for the thorough review.
I believe that is accurate and captures the current behavior in v1.7
@jkowalleck I know you worked on this. What are your thoughts. I attempted to keep the existing logic in place - and I think I have.
I'm open to improving the wording. Suggestions. As far as the design goes. This issue passes the public RFC comment period. |
|
Actually, this was a defect, so no comment period was necessary. |
Thanks — on wording, I think it really depends on whether we’re just clarifying the text or actually fixing the defect. If we fix the defect and require • Rule: “ If, however, we keep the current behavior (where • “If Happy to go with whichever direction makes the most sense here. |
|
Your current approach makes the spec messy with all those constraints glued on the thing. alternative approach: after the current
after that split, the current put this idea into a ticket: #1024 |
| "description": "Requirement: 'versionRange' must not be present when 'isExternal' is `false`.", | ||
| "if": { | ||
| "properties": { "isExternal": { "const": false } } | ||
| "required": ["versionRange"] |
There was a problem hiding this comment.
intention:
? Requirement: 'versionRange' must not be present when 'isExternal' is false.
before change:
if isExternal is set to false, then versionRange must not be present.
-or- only if isExternal is true, then versionRange may be set.
| isExternal | versionRange present | result/valid per rule |
|---|---|---|
| 0 | 0 | 1 |
| 0 | 1 | 0 |
| 1 | 0 | 1 |
| 1 | 1 | 1 |
after change:
if versionRange is set, then isExternal must be true
-or- only if isExternal is true, then versionRange may be set.
| isExternal | versionRange present | result/valid per rule |
|---|---|---|
| 0 | 0 | 1 |
| 0 | 1 | 0 |
| 1 | 0 | 1 |
| 1 | 1 | 1 |
while technically the "before" and "after" work the same, i find the "bnefore" better since it is more on the actual textuial description>
There was a problem hiding this comment.
Agreed that "before" and "after" are logically equivalent as truth tables — that part isn't in question. My preference for "after" (versionRange present → isExternal must be true) is really about consistency: the versionRange field description already says "May only be used if isExternal is true," which is that same direction. Wording the rule description the other way just means a reader has to mentally contrapose one against the other to confirm they agree.
Both truth tables treat isExternal as strictly binary (true/false), but in the schema it can also be absent. The current if block keys on "isExternal": {"const": false}, which won't match a missing property — so an object with versionRange set and isExternal omitted entirely still passes validation today. That's the actual defect I was pointing at originally, not just the wording. Whichever phrasing we land on, I think the spec text and the schema logic both need to explicitly account for the "isExternal omitted" case, not just true/false.
Implements and closes #638