Skip to content

Conversation

@ubmarco
Copy link
Member

@ubmarco ubmarco commented Jul 8, 2025

This is a follow-up on #1441.
I plan not to merge #1441 and merge this PR instead.

Differences:

  • Aligning with standards in JSON schema for re-using subschemas via $defs and $ref
  • Fully typed implementation including runtime checks of valid schema user input
  • Auto inject the default string type if not given
  • Replace trigger_schema with select which aligns with query language terminology
  • Replace trigger_schema_id with the mentioned $ref mechanism
  • New schemas root key validate with sub-keys local and network for the 2 validation types
  • Network validation items does not allow the select key anymore as the selection happens by linking target needs.
    This cleans up an ambiguity in the other PR.
  • Network validation errors now bubble up to the root json schema and are displayed to see exactly why the chain fails
  • More network rule types for better control over debug schema output
  • Rewrite test cases to use a declarative definition as yaml, so all pieces can be given in one place:
    • conf.py
    • ubproject.toml
    • index.rst
    • schemas.json
    • expected ontology warnings
  • Simplified the code logic
  • String patterns (regex) are constrained to a basic subset that works across engines
  • Added docs
    • Examples and explanations
    • Comparison with needs_warnings and needs_constraints and migration path
  • Many more test cases
  • items with minItems and maxItems and contains with minContains and maxContains are now semantically equivalent to JSON schema spec

@ubmarco ubmarco mentioned this pull request Jul 9, 2025
@ubmarco ubmarco force-pushed the mh-schema-validation-v2 branch 3 times, most recently from 4b4057b to 26cae92 Compare July 9, 2025 09:07
@codecov
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

❌ Patch coverage is 90.23904% with 98 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.81%. Comparing base (4e10030) to head (3ca68e0).
⚠️ Report is 186 commits behind head on master.

Files with missing lines Patch % Lines
sphinx_needs/schema/reporting.py 75.71% 34 Missing ⚠️
sphinx_needs/schema/config_utils.py 89.57% 27 Missing ⚠️
sphinx_needs/schema/core.py 92.77% 19 Missing ⚠️
sphinx_needs/builder.py 57.89% 8 Missing ⚠️
sphinx_needs/needs.py 80.55% 7 Missing ⚠️
sphinx_needs/schema/process.py 93.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
+ Coverage   86.87%   88.81%   +1.93%     
==========================================
  Files          56       67      +11     
  Lines        6532     8271    +1739     
==========================================
+ Hits         5675     7346    +1671     
- Misses        857      925      +68     
Flag Coverage Δ
pytests 88.81% <90.23%> (+1.93%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ubmarco ubmarco requested a review from chrisjsewell July 10, 2025 17:03
@ubmarco ubmarco force-pushed the mh-schema-validation-v2 branch 5 times, most recently from 0d3e8c5 to 6e95ec0 Compare July 13, 2025 20:48
@ubmarco ubmarco force-pushed the mh-schema-validation-v2 branch from 6e95ec0 to a08078c Compare July 13, 2025 20:58
@ubmarco ubmarco marked this pull request as ready for review July 13, 2025 21:05
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review comments

(without yet looking over the full implementation of the schema checking)

Comment on lines +60 to +61
The option will still be stored as a string in Sphinx-Needs,
but during schema validation, the value will be coerced to the given type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bad though, the values should be coerced to these from the start, if read from a (string) directive, or just checked if read from a needs.json.
This will definitely be the case in ubcode.
What is the path to change sphinx-needs, so that it can handle storing the data as the correct type?

Copy link
Member Author

@ubmarco ubmarco Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with you that the internal types should change with the definition, my plan was to do that in a 2-step approach. Users should adopt the ontology in a non-breaking way (switch from warnings/constraints to this, transpile from SHACL, ...). This takes time and efforts. Then in the meantime we change the internal representation which affects at least filter functions, and data/schema in exported needs.json. My current feeling is changing the types is a bigger thing in the codebase that delays this PR. The adoption of the new interface and the schema feedback is already of good value for end users. We can switch the type system while adoption happens.

ubmarco and others added 8 commits July 16, 2025 21:47
Make get_warnings_list a fixture.
Co-authored-by: Chris Sewell <[email protected]>
- Renamed `needs_schemas` to `needs_schema_definitions`
- Fail if both config needs_schema_definitions and json is given
@ubmarco ubmarco requested a review from chrisjsewell July 20, 2025 21:51
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment, emit a warning

then ok thats it for now; this PR can be merged, but there will be changes that need to be made when I get into it

@ubmarco ubmarco requested a review from chrisjsewell July 24, 2025 14:27
@ubmarco ubmarco merged commit 9d86602 into master Jul 24, 2025
20 checks passed
@ubmarco ubmarco deleted the mh-schema-validation-v2 branch July 24, 2025 19:25
PierreEtienneJ pushed a commit to PierreEtienneJ/sphinx-needs that referenced this pull request Jul 29, 2025
This is the implementation of the discussion
useblocks#1451
and a follow-up on useblocks#1441.

The PR adds schema validation to Sphinx-Needs that
supports local validation and network validation.
The Sphinx-Needs internal data type representation is not changed yet,
all types are still strings. This will come shortly after merge.
Users can already try out the new interface and provide feedback.

Differences to PR 1441:
- Aligning with standards in JSON schema for re-using subschemas via
`$defs` and `$ref`
- Fully typed implementation including runtime checks of valid schema
user input
- Auto inject the default string type if not given
- Replace `trigger_schema` with `select` which aligns with query
language terminology
- Replace `trigger_schema_id` with the mentioned `$ref` mechanism
- New schemas root key `validate` with sub-keys `local` and `network`
for the 2 validation types
- Network validation items does not allow the `select` key anymore as
the selection happens by linking target needs.
  This cleans up an ambiguity in the other PR.
- Network validation errors now bubble up to the root json schema and
are displayed to see exactly why the chain fails
- More network rule types for better control over debug schema output
- Rewrite test cases to use a declarative definition as yaml, so all
pieces can be given in one place:
  - conf.py
  - ubproject.toml
  - index.rst
  - schemas.json
  - expected ontology warnings
- Simplified the code logic
- String patterns (regex) are constrained to a basic subset that works
across engines
- Added docs
  - Examples and explanations
- Comparison with `needs_warnings` and `needs_constraints` and migration
path
- Many more test cases
- `items` with `minItems` and `maxItems` and `contains` with
`minContains` and `maxContains` are now semantically equivalent to JSON
schema spec

---------

Co-authored-by: Chris Sewell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants