sul-dlss / cocina-models

Cocina repository data model (implemented in Ruby)
https://sul-dlss.github.io/cocina-models/
3 stars 0 forks source link

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

Closed jmartin-sul closed 10 months ago

jmartin-sul commented 10 months ago

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? 🤨

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

jmartin-sul commented 10 months ago

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

jmartin-sul commented 10 months ago

fixed the method i ripped off to be more concise, using the suggestion from this PR: https://github.com/sul-dlss/cocina-models/pull/645