search

Altinn / altinn-studio

Next generation open source Altinn platform and applications.
https://docs.altinn.studio
BSD 3-Clause "New" or "Revised" License
111 stars 70 forks source link

Using the same name for a data model and a FileUpload/FileUploadWithTag component should not be allowed #13172

Open mlqn opened 2 months ago

mlqn commented 2 months ago

Description of the bug

Using the same name for a data model and a FileUpload/FileUploadWithTag component should not be allowed, as they will both have the same ID in applicationmetadata.json and they can both be used as dataTypesToSign on signing tasks.

If duplicate IDs in applicationmetadata.json are not an issue, then we might just need to specify whether it is a model or a FileUpload/FileUploadWithTag in the dataTypesToSign select.

An easy solution could also be to add a suffix to the IDs.

Another solution could be to check if the name already exists in applicationmetadata.json. This solution could also fix #13171.

Steps To Reproduce

https://github.com/user-attachments/assets/cc9e38bb-5fcc-4f69-99df-7bfba7460235

Additional Information

No response

nkylstad commented 1 month ago

I suggest we use the suggestion of checking against existing id's in applicationMetadata dataTypes. Duplicate ID's for data types are not allowed.

standeren commented 5 days ago

Technically the issue is fixed 👏

But I feel that the error messages should be more descriptive:

  1. When I name a new datamodel for the same as a fileuploader I get the message "Modellnavnet er allerede i bruk" which is correct, but when I get the message in the datamodel-context it is interpret that a model already have that name. So maybe a better message would be "Modellnavnet er allerede bruk som en ID for en annen datatype" eller "Modellnavnet er allerede i bruk som en ID for en filopplastningskomponent". The last one is a "mouthfull" tho 😅
  2. When assigning a file uploader the name of a model I get the message; "Komponenten mĂ„ ha en unik ID" which is also correct, but maybe more informative to say; "Denne ID-en er allerede i bruk for en datamodell" đŸ€·â€â™€

Maybe @Ildest has some input in this case

Ildest commented 5 days ago

Technically the issue is fixed 👏

But I feel that the error messages should be more descriptive:

  1. When I name a new datamodel for the same as a fileuploader I get the message "Modellnavnet er allerede i bruk" which is correct, but when I get the message in the datamodel-context it is interpret that a model already have that name. So maybe a better message would be "Modellnavnet er allerede bruk som en ID for en annen datatype" eller "Modellnavnet er allerede i bruk som en ID for en filopplastningskomponent". The last one is a "mouthfull" tho 😅
  2. When assigning a file uploader the name of a model I get the message; "Komponenten mĂ„ ha en unik ID" which is also correct, but maybe more informative to say; "Denne ID-en er allerede i bruk for en datamodell" đŸ€·â€â™€

Maybe @Ildest has some input in this case

Hei! PÄ 1 tenker jeg at det fÞrste forslaget er good to go, men at vi prÞver Ä unngÄ det andre, hvis vi ikke kan si "...ID for filopplasting" bare @standeren?. PÄ 2 gÄr jeg for 2 :-).

standeren commented 5 days ago

Takk for respons đŸ„ł Jeg heller kanskje mer mot "Modellnavnet er allerede i bruk som ID for filopplasting" for Ă„ tydeliggjĂžre at den er i bruk et annet sted enn pĂ„ datamodell-siden.

Jondyr commented 5 days ago

@standeren Jeg er enig i at meldingene kunne vÊrt mer beskrivende. Problemet er at vi bruker applicationmetadata.json til Ä sjekke duplikater, og her er det ikke en enkel mÄte Ä vite typen til objektet som kolliderer. Det er mulig man kunne funnet typen ved Ä kryssjekke ID-en med andre kilder, eller se pÄ strukturen til objektet, men jeg fÞler det blir mye arbeid for en litt mer beskrivende feilmelding. Hva tenker du?

standeren commented 5 days ago

@Jondyr Kunne man sjekket at Id-en man skrev inn ikke har dette feltet i objektet? 

"allowedContentTypes": [
--
  | "application/xml"
  | ]

Og pĂ„ Utforming-siden er det en egen sjekk pĂ„ om id-en eksisterer i appmetadata sĂ„ der kan vi vel eksplisitt gi en annen feilmeldingen om det er den eller idExists sjekken som treffer? 😊

Jondyr commented 5 days ago

Kunne man sjekket at Id-en man skrev inn ikke har dette feltet i objektet?

Da har man fremdeles flere muligheter for hva typen er enn en filopplasting, utifra min forstĂ„elseđŸ€”Og om det skulle komme nye komponenter med datatyper vil det Ă„ kalle det "filopplasting" bli feil

Og pĂ„ Utforming-siden er det en egen sjekk pĂ„ om id-en eksisterer i appmetadata sĂ„ der kan vi vel eksplisitt gi en annen feilmeldingen om det er den eller idExists sjekken som treffer? 😊

Her ogsÄ vil det kunne vÊre flere ting som kolliderer enn bare en datamodell, mener jeg

standeren commented 4 days ago

Kunne man sjekket at Id-en man skrev inn ikke har dette feltet i objektet?

Da har man fremdeles flere muligheter for hva typen er enn en filopplasting, utifra min forstĂ„elseđŸ€”Og om det skulle komme nye komponenter med datatyper vil det Ă„ kalle det "filopplasting" bli feil

Og pĂ„ Utforming-siden er det en egen sjekk pĂ„ om id-en eksisterer i appmetadata sĂ„ der kan vi vel eksplisitt gi en annen feilmeldingen om det er den eller idExists sjekken som treffer? 😊

Her ogsÄ vil det kunne vÊre flere ting som kolliderer enn bare en datamodell, mener jeg

Kanskje man kan si; "Modellnavnet er allerede brukt for andre IDer i lĂžsningen" for Ă„ eksplisitt formidle at det ikke bare gjelder datamodell? Samt, "Komponenten kan ikke ha samme id som datatyper i lĂžsningen" for komponenten? Eller noe i den duren

Ildest commented 4 days ago

Kunne man sjekket at Id-en man skrev inn ikke har dette feltet i objektet?

Da har man fremdeles flere muligheter for hva typen er enn en filopplasting, utifra min forstĂ„elseđŸ€”Og om det skulle komme nye komponenter med datatyper vil det Ă„ kalle det "filopplasting" bli feil

Og pĂ„ Utforming-siden er det en egen sjekk pĂ„ om id-en eksisterer i appmetadata sĂ„ der kan vi vel eksplisitt gi en annen feilmeldingen om det er den eller idExists sjekken som treffer? 😊

Her ogsÄ vil det kunne vÊre flere ting som kolliderer enn bare en datamodell, mener jeg

Kanskje man kan si; "Modellnavnet er allerede brukt for andre IDer i lĂžsningen" for Ă„ eksplisitt formidle at det ikke bare gjelder datamodell? Samt, "Komponenten kan ikke ha samme id som datatyper i lĂžsningen" for komponenten? Eller noe i den duren

Hei @standeren :-). Den fÞrste setningen er ok, bare pass pÄ Ä skrive ID-er, med bindestrek. I den andre bruk ID med store bokstaver. Ellers har jeg ikke noen tilbakemeldinger pÄ disse :-).

Jondyr commented 1 day ago

@standeren @Ildest

"Modellen kan ikke ha samme navn som datatyper i lĂžsningen" "Komponenten kan ikke ha samme ID som datatyper i lĂžsningen"

Er disse to ok?

standeren commented 19 hours ago

Nice! Dette ble veldig eksplisitt đŸ„ł

Men jeg oppdaget en annen ting; man kan fortsatt laste opp en datamodell med samme Id som en vedleggskomponent...

Jeg har gjort en liten refaktorering av opplastingskomponenten ifm bildeopplastingsPRen min. Der sender jeg inn en valideringsfunksjon for filnavn hvor vi blant annet kan si at filnavnet man laster opp ikke kan vÊre lik en liste filnavn og at det mÄ matche en viss regex.

https://github.com/Altinn/altinn-studio/pull/13322/files#diff-910e5324f04bd255611c0fb2d41ce324ff9cb43652c0165babd03ae287fc7ec2R68

Usikker pÄ om denne lenken tar deg til riktig sted. Hvis ikke, kan du se pÄ XsdFileUpload fila i den PRen.

Er nok fortsatt en stund til den PRen gĂ„r igjennom sĂ„ vi kan evt lage en ny sak pĂ„ Ă„ legge til den type validering sĂ„ snart den PRen er igjennom? Eller hva tenker du? 😊

Evt kan jeg skille ut den endringen fra den PRen i en egen PR?