-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Tighten validation for id fields (followup) #8069
Merged
amercader
merged 40 commits into
master
from
shashigharti-7604/tighten-validation-for-id-fields
May 23, 2024
Merged
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
0ff9e93
Add validation and test to Resource
shashigharti 482b796
Fix for failing test
shashigharti 19937f0
Rename resource_id_validator function to id_validator
shashigharti 7669403
Add validation for Resource View
shashigharti df1e66f
Add validator to check unique id value
shashigharti fd771ce
Add validation to group
shashigharti 1b580c3
change test name
shashigharti 3308546
Added validation for package extra
shashigharti 7d6c4de
Added id validation for group extras
shashigharti f08e4d8
Add validation in tag
shashigharti 946a07d
Add id validator for activity
shashigharti a230be1
Merge branch '7604/tighten-validation-for-id-fields' of https://githu…
amercader 4f85b61
Add id_validator to datasets
amercader 0c9daaa
Tag ids can not be set by users
amercader dcf84e8
Activity ids can not be set via the API
amercader 393e17f
Consolidate order of the validators
amercader e3d6200
Separate validator for user id
amercader 0ed63cc
Separate schema for extras show
amercader bb8e533
Better message for id validator
amercader 9fbd0b9
Prevent further validation on empty_if_not_sysadmin
amercader 18c19f5
No id validation on group show
amercader 09c00c9
User id or name when updating
amercader 6af7cd0
Fix validators for resource update
amercader ea1d824
Fix id length in test
amercader c48bc71
Enforce actual UUIDs as ids
amercader 22e4cb5
Fix more uuid tests
amercader 8922684
Simplify schema methods in IGroupForm
amercader 24da1f1
Fix more uuid tests
amercader 3eca61b
Fix schema for user form
amercader 5868a7b
Remove empty_if_not_sysadmin from resource ids
amercader 920a8df
Lint
amercader dc6bbad
Add changelog
amercader d9968bb
Merge branch 'master' into shashigharti-7604/tighten-validation-for-i…
amercader 48cde8e
Re-add schema calls lost during last merge
amercader 0c7daf9
Add schema methods to IGroupForm
amercader 3758551
lint
amercader a7e16b1
Changelog tweaks
amercader bd720fd
Fall back to old IGroupForm methods for compatibility
amercader 517a8ee
lint
amercader dbbf0fa
Better plugin schema methods checks
amercader File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
* Only sysadmins can now set the ``id`` field of Datasets, Groups, Organizations, Users, Resource Views and Extras | ||
* If provided, the value of the ``id`` field needs to be a valid UUID v4 string. Sites using custom ids that are not UUIDs can extend the relevant schema or validate methods to override the validation on the ``id`` field, but are strongly encouraged to use a separate custom field to store the custom id instead. | ||
* The ``form_to_db_*`` and ``db_to_form_*`` methods of the ``IGroupForm`` interface are now deprecated, and have been replaced by``create_group_schema()``, ``update_group_schema()`` and ``show_group_schema()``. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only deprecated, these won't be called at all any more even as a fall-back, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(looks like ckanext-scheming uses the
validate
method ofIGroupForm
it won't be affected by the change)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I thought of leaving them for a version in case some plugin was using them but that's seems unlikely and maybe we can just remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polite thing would be to have a version released with the methods deprecated and working as a fall-back then remove them next release.
Marking them deprecated but actually broken doesn't make sense. Better to remove them and raise an error when a plugin tries to use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure @wardi , you are suggesting that in the actions, if an
IGroupForm
does not have one of the new methods (egIGroupForm.create_group_schema()
it should try to call the old equivalent, egIGroupForm.form_to_db_schema()
right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, with the deprecation warning and remove it the next release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wardi see bd720fd