Feat/methods to access xsd#1668
Feat/methods to access xsd#1668MagnusNordboe wants to merge 3 commits intoAltinn:feat/methods-to-access-xsdfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ivarne
left a comment
ivarne
left a comment
There was a problem hiding this comment.
Ser bra ut. Har noen små kommentarer. I tillegg må vi nok ha noen unit tester så vi ikke brekker funksjonaliteten når vi gjør endringer.
| continue; | ||
| } | ||
| var formData = await dataAccessor.GetFormData(dataElement); | ||
| ObjectUtils.RemoveAltinnRowId(formData); |
There was a problem hiding this comment.
Bør vi fjerne altinnRowId her, eller bare kreve at den legges til som optional i xsd? Det er nok flere forskjeller, men dette er jo det eneste eksemplet (jeg har sett det har blitt klaget på) der xsd validering og data annotations gir ulikt resultat, men kanskje dere har andre grunner for å gjøre dette?
There was a problem hiding this comment.
I testing, så arresterer validatoren deg om du har rowId og det ikke er i Xsd.
Det er ingen enkel måte å si blankt "Alle elementer kan ha AltinnRowId"; man må legge det til på hvert enkelt element, og det blir mye rot i Xsdene. For vårt prosjekt ville det vært ~1000 endringer.
Jeg har snakket med produkteieren vår angående altinnRowId og fått tydelig beskjed at vi ikke skal forholde oss til det i Xsd. Så her har egen erfaring blitt med inn som en beslutning i design av løsningen.
|
|
||
| if (appSettings?.XsdValidation is true) | ||
| { | ||
| services.AddTransient<IValidator, XsdValidator>(); |
There was a problem hiding this comment.
Bør det være standard å ha både DataAnnotationValidator og XsdValidator, eller bør aktivering av XsdValidation være et alternativ til dataAnnotations?
There was a problem hiding this comment.
Jeg kan ikke si noe veldig konsekvent, men kan dele vår erfaring med disse typene validering.
Vi har opplevd at en god del innsendinger har blitt godkjent av DataAnnotationValidator, deretter kommet til backend som kjører xsd validering og klager på innsendingen. Så det virker som xsd validering er strengere enn DataAnnotationValidator. Vi kjører alltid xsd datamodeller gjennom altinn studio sitt generer datamodeller-verktøy.
There was a problem hiding this comment.
Jeg har også grublet litt over om xsd validering burde være standard. Jeg føler det finnes et sterkt argument for, og et sterkt argument mot.
For: siden datamodeller kan genereres ut fra Xsd, kan man ofte anta at hvis man har en Xsd med samme navn som en modell, i models mappa, så vil man gjerne validere dataen utfra den xsden. og hvis man utvikler og finner at det skaper problemer, kan man deretter slå av. Ref det jeg nevnte over, med at xsd validering virker som det er strengere. Dette hadde vært "least trust" prinsipp, som noen setter pris på.
Imot: Jeg vet om hvertfall ett prosjekt, som startet i utgangspunktet med datamodell generert ut fra Xsd, og deretter gikk over til å bare redigere .cs filene direkte når de gjorde endringer i modellene. Jeg vet ikke hvor vanlig dette er, men slike utviklere vil helst ikke ha denne funksjonaliteten som standard.
3 participants
Uses the new methods to access XSD to add opt-in validation based on XSD rules, if user has a schema with the same name as the data model.
Description
XsdValidationVerification
Documentation