refactor: replace patternProperties with additionalProperties for imp…#323
refactor: replace patternProperties with additionalProperties for imp…#323jack-berg merged 2 commits intoopen-telemetry:mainfrom
Conversation
…roved type safety - Replace `"additionalProperties": true` with typed `additionalProperties` schemas - Remove redundant `patternProperties` in favor of `additionalProperties` - Update wildcard pattern matching from ".*" to ".+" for more precise matching - Affects schema files: instrumentation, logger_provider, meter_provider, propagator, resource, tracer_provider - Update documentation and script comments to reflect pattern change from ".*" to ".+" This change improves JSON schema validation by providing explicit typing for additional properties instead of allowing any properties, while maintaining backward compatibility for configuration extensions.
dol
force-pushed
the
fix/wildcard-pattern
branch
from
1de0fa6 to
fc38482
Compare
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Small comment about .instrumentation, but I think this is an improvement. Thanks!
| "type": "object", | ||
| "additionalProperties": false, | ||
| "additionalProperties": { | ||
| "$ref": "#/$defs/ExperimentalLanguageSpecificInstrumentation" |
There was a problem hiding this comment.
Do you have another language in mind that needs to be represented and is not in the list of properties? If not, let's disallow additional properties until a use case emerges.
There was a problem hiding this comment.
I agree that if we dont have a language yet, we should disallow it. We can always add languages without breaking changes
There was a problem hiding this comment.
I don't have any other languages in mind. I was just migrating
to theadditionalProperties level.
I'm hesitant to change the existing behavior by disallowing additional languages. I rather prefer creating a new pull request to get rid of the .* rule in the first place and then not include it in this pull request, which then will be merged later.
Current schema
Pull request schema
Credits: https://jsonviewer.tools/editor was used to visualize the schema definition
There was a problem hiding this comment.
Oh I see what you're saying. The current schema is contradictory. It has additionalProperties: false AND patternProperties: .*. I.e. "you're not allowed to have extra properties, but if you do, they need to conform to this schema". Nonsense!
I just ran a test locally and it appears that the patternProperties: .* is taking priority over additionalProperties: false, which is surprising to me.
But I agree with your conclusion. Let's get this PR merged as is, and open a followup to disallow additional languages (i.e. the intent of the current schema).
There was a problem hiding this comment.
Since patternProperties: .* has presidency over additionalProperties it's really misleading and can lead to unwanted behaviors.
Thank you for testing this.
The goal of this pull request was to get rid of this ambiguous situation. Thx for merging it.
4 participants
…roved type safety
"additionalProperties": truewith typedadditionalPropertiesschemaspatternPropertiesin favor ofadditionalPropertiesThis change improves JSON schema validation by providing explicit typing for additional properties instead of allowing any properties, while maintaining backward compatibility for configuration extensions.
JSON Schema docs: https://json-schema.org/understanding-json-schema/reference/object#additionalproperties
Example: https://github.com/puremourning/vimspector/blob/0da16c67e5fc862ac8988d0580a58d40858f2456/docs/schema/vimspector.schema.json#L278