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

switch LanguageTagValidator to operate on DRO::TYPES so that it's invoked automatically when building item models #643

Merged
merged 1 commit into from Nov 8, 2023

Conversation

jmartin-sul
Copy link
Member

@jmartin-sul jmartin-sul commented Nov 8, 2023

Why was this change made? 🤔

this makes LanguageTagValidator operate more like DarkValidator, which we determined in slack discussion is likely desirable for real world validation usage:

@mjgiarlo 💬

What I think is happening: only some models use the ruby validation code (items, collections, APOs), and I don't think files are in that list. So they're strictly openapi validated.

We likely need to rewrite the language tag validator so that it works against items instead of against files (so it runs at the item level and descends down into files, just like the dark-access validator)
...
(and we can't lean on openapi validation for BCP unless we want to couple a given cocina-models release to a an enum of allowed BCP values, which is maybe a bit brittle)
...
It's a pretty small code change for a cocina-models patch, I think
to rework the language tag validator to operate against DROs like the https://github.com/sul-dlss/cocina-models/blob/35ef3bdf826f53e7fd6e158b2db3b85a8cfadd78/lib/cocina/models/validators/dark_validator.rb

@jmartin-sul 💬

ah, ok, that makes sense. didn't realize validation would be invoked automatically on the argo side for only some model types, and there wasn't any overarching info needed at the item level (i don't think?) to validate the tag on an individual file, so i went for the simpler file level thing.

@mjgiarlo 💬

validation in cocina-models is maybe a bit surprising in this way. it's like that in the gem, too, not just argo. validation is wired up on the instance and class new methods: https://github.com/sul-dlss/cocina-models/blob/main/lib/cocina/models/validatable.rb
but not every model uses the Validatable module. (DRO does. File doesn't.)

How was this change tested? 🤨

  • unit tests
  • tweak the argo branch for this PR to use this cocina-models branch, deploy to stage, and see if that fixes the lack of validation on languageTag mentioned in the linked slack discussion? and if so, merge this PR and cut a new patch release and update the argo PR to use that?

⚡ ⚠ If this change has cross service impact, run integration tests and/or test in [stage|qa] environment, in addition to specs. ⚡

@jmartin-sul jmartin-sul force-pushed the makeLanguageTagValidatorOperateAtDroLevel branch from 813256a to 1a7f7e8 Compare November 8, 2023 19:56
@jmartin-sul
Copy link
Member Author

@justinlittman and @mjgiarlo thanks for the feedback, implemented all suggestions, ready for another look

@jmartin-sul jmartin-sul force-pushed the makeLanguageTagValidatorOperateAtDroLevel branch from 1a7f7e8 to 6c1aec4 Compare November 8, 2023 20:03
jmartin-sul added a commit that referenced this pull request Nov 8, 2023
…re concise/idiomatic

cribbed off this class for LanguageTagValidator, switched to something more concise for a similar method there, doing the same here

see #643
@jmartin-sul
Copy link
Member Author

fixed the method i ripped off to be more concise, using the suggestion from this PR: #645

…oked automatically when building item models
@jmartin-sul jmartin-sul force-pushed the makeLanguageTagValidatorOperateAtDroLevel branch from 6c1aec4 to 5ab9875 Compare November 8, 2023 21:03
@mjgiarlo mjgiarlo merged commit 2ea131b into main Nov 8, 2023
6 checks passed
@mjgiarlo mjgiarlo deleted the makeLanguageTagValidatorOperateAtDroLevel branch November 8, 2023 21:06
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.

None yet

3 participants