Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect and collect deprecated fields in the check #24

Merged
merged 9 commits into from
Oct 14, 2024
Merged

Conversation

janvhs
Copy link
Member

@janvhs janvhs commented Oct 10, 2024

This PR allows to collect and show deprecations in the checks. For more information and reasoning behind changes, have a look at the commit message. (I wish I could just tell GitHub to throw these in here)

This pulled in a bunch of crates we do not need. A few of these are
crates for networking and async
This comment provides a function to collect deprecated fields from the
given check, so they can be logged as an error.

Due do the limitations of jsonschema-rs, it is not possible to log
deprecations if the given schema is not valid otherwise.  I wish to
address this via using <https://github.com/santhosh-tekuri/boon> instead
of committing this to the upstream project, since the API of boon is
way better.

Furthermore, it is not possible to detect deprecations in linked types
($ref), due to shortcomings in the used crate.  Currently this
functionality is not needed and might be addressed via moving to boon
It is not necessary to utilise the Result type here and complicates the
code.  Therefore, I removed it while implementing the collection of
deprecations
There was some unformatted code in the upstream branch.  Maybe having a
check for this could come in handy
@janvhs janvhs requested a review from dottorblaster October 10, 2024 10:53
@janvhs janvhs marked this pull request as ready for review October 10, 2024 14:34
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

I like the approach catching every deprecated field, I would have aimed for something way simpler. Also, I would prefer the deprecation thing to be another validator, not modifying too much what we already had in place.

Can you fix the tests?

Copy link

@gagandeepb gagandeepb left a comment

Choose a reason for hiding this comment

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

  1. Have you considered locking down on a more recent jsonschema meta-schema version ? I understand more recent versions have a richer validation/schema-evolution capabilities.
  2. I wonder if it's interesting/feasible to express deprecation of fields using the schema-evolution capabilities of more recent versions. 🤔

@janvhs
Copy link
Member Author

janvhs commented Oct 10, 2024

Yes I did, but updating the crate is not required, since it does not implement handling deprecations in future releases. As stated in the commit messages, we're going to replace this crate in future releases with boon, for it actually covers modern specs and bumping the crate to 0.20 requires updating the code either way.

@gagandeepb
Copy link

gagandeepb commented Oct 11, 2024

I was not suggesting to update the crate, necessarily. However, I am/was under the impression that more recent versions of the json schema spec (not the crate, but the "dialect" that a specific schema instantiation adheres to) support field-level deprecation (according to the link I shared previously). I also understand that newer versions of the jsonschema crate support newer dialects (like draft2020, draft2019, etc.) but I am not fully aware of specific dialect support/conformance limitations of the current jsonschema crate. Concretely, I was wondering :

  1. If updating the schema dialect of the underlying checks DSL spec to something more recent, and
  2. If marking specific fields in the DSL spec as deprecated, and
  3. If handling 1 and 2 here in this repo (i.e. tlint) by making use of whichever implementation of json schema validation that appropriately conforms to the dialect that supports field-deprecation at the schema level. ...
    ... is something that is feasible/interesting to evaluate for the purposes here?

@janvhs
Copy link
Member Author

janvhs commented Oct 11, 2024

json schema spec (not the crate, but the "dialect" that a specific schema instantiation adheres to) support field-level deprecation

Yeah they do and that is what is leveraged here

I am not fully aware of specific dialect support/conformance limitations of the current jsonschema crate

TLDR: they don't support this

If updating the schema dialect of the underlying checks DSL spec to something more recent, and

Sure, I can replace the link, but it won't change the fact that this implementation would still be required

If marking specific fields in the DSL spec as deprecated, and

I don't think it is wise to have multiple sources of truth

As reference af224c0

@gagandeepb
Copy link

If marking specific fields in the DSL spec as deprecated, and

I don't think it is wise to have multiple sources of truth

Indeed it is not, I agree. But how is marking specific fields in the DSL spec as deprecated an instance of multiple sources of truth? Isn't the spec supposed to be the source of truth and tlint the "verifier"/checker of an instantiation of a check against the spec?

@janvhs janvhs merged commit 85ea848 into main Oct 14, 2024
2 checks passed
@janvhs janvhs deleted the add-deprecation-logs branch October 14, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants