microsoft / AdaptiveCards

A new way for developers to exchange card content in a common and consistent way.
https://adaptivecards.io
MIT License
1.77k stars 553 forks source link

Json generated from designer not valid because of upper/lower case for enums #2393

Closed ilylin closed 5 years ago

ilylin commented 5 years ago

Platform

Author or host

Are you an author (like sending something to Outlook), or a host that is rendering your own cards?

If you're an author, who are you sending cards to?

Version of SDK

What version are you using? Ex: NuGet 1.0.2, or latest master, etc...

Issue

Cards generated from designer have uppercase enum values (such as 'Medium' or 'Small' for image size), while the schema requires lowercase values. This leads to failed json schema validation

dclaux commented 5 years ago

@matthidinger and @khouzam, Adaptive Cards is supposed to be case insensitive when it comes to enum values. Is the iOS renderer too restrictive?

ilylin commented 5 years ago

I was using everit json schema validator: https://github.com/everit-org/json-schema passed json from the designer and schema from here: https://adaptivecards.io/schemas/adaptive-card.json

It didn't validate until I changed all the uppercase enums to lowercase

dclaux commented 5 years ago

@ilylin JSON is case sensitive by nature, and as far as I know it is not possible to define a case-insensitive enumeration in a JSON schema. So what you're seeing is normal. But it remains that Adaptive Cards enumerations are case insensitive. You can check in the designer: "small", "Small" or "SmAlL" are all accepted.

ilylin commented 5 years ago

@dclaux Right, it is case-insensitive and the adaptive card schema defines them as all lowercase: "enum": [ "auto", "stretch", "small", "medium", "large" ] However, a card that my teammate created in adaptive card designer had capitalized values and that json did not pass the validation. IMO designer should produce lowercase values that comply with schema. Makes sense?

ilylin commented 5 years ago

See example card here: https://adaptivecards.io/designer/ "type": "TextBlock", "size": "Medium", "weight": "Bolder",

Note that Medium and Bolder are capitalized

dclaux commented 5 years ago

Ah, I did miss your point. Probably because IMO the solution is to somehow create a schema that is case-insensitive when it comes to enums, not constrain the designer. We've already established that cards authored by hand do not respect enum casing most of the time, which is why we decided to make enum values case-insensitive.

Let us look into this.

dclaux commented 5 years ago

So I was looking here and we could actually define our enum properties as strings using regular expressions patterns instead of enums.

Unfortunately, JSON schema doesn't support case-insensitive matches even for regular expressions. So we'd have to define each value as something like this ^[Ss][Mm][Aa][Ll][Ll]$ and that's just for "Small". But at least it's an option.

ilylin commented 5 years ago

Thanks! Yes, JSON schema doesn't support case insensitive unfortunately. Would it be possible to change the generators in the designer to produce values that are compliant with schema (i.e. lowercase)?

dclaux commented 5 years ago

Maybe I wasn't clear - I don't think we want to constrain the designer; we want to relax the schema, and there is a (somewhat convoluted) way to define case insentive enum values in a JSON schema as per I wrote above.

Going all lowercase isn't enough ("extraLarge" for example has an upper case L) and manually written payloads that do not use proper casing would still vail validation even though they are correct.

What I can probably do quickly is change the first letter of all enum values to lower case, that might do the trick.

ridder-mark commented 5 years ago

@dclaux is there any planned exercise to relax the schema as you mentioned above in the upcoming version?

matthidinger commented 5 years ago

@ridder-mark I have some planned schema improvements in the works, but it likely won't be published for a few weeks.

@dclaux I think if it's easy enough for you to lowercase the first letter for now that would be a good work around. Keep in mind for me to do a site deploy it'll have to be from the 1.x codebase, so once you've done this change we could cherry-pick it into the release/1.1 branch hopefully

ridder-mark commented 5 years ago

This is a reasonable workaround. I just wish we can save other people's time by somehow publishing this somewhere.

ilylin commented 5 years ago

@dclaux @ridder-mark @matthidinger Any update on this?

adampauls commented 5 years ago

Hi. I'm on @ilylin's team, and also have an interest here. I didn't quite follow why changing the designer to produce lowercased values by default is going to cause problems? It wouldn't "constrain" the designer -- users could still edit the values to be uppercased if they want. Or maybe you mean you don't want to constrain the implementation of the designer?

That said, if changing the schema to be case-insensitive on the first letter is the easiest thing to do (which I believe is the proposed workaround?), that's great too!

dclaux commented 5 years ago

@adampauls not all enum values in Adaptive are all lower case. For instance, extraLarge for text size. The designer can't simply emit all enum values in lower case, it would have to emit them as defined in the schema.

Also, it is not the designer that serializes to JSON, it is the renderer. Emitting enum values per the schema would mean implementing logic in the renderer to dynamically load the schema and map its own enum definitions to the way they're defined in the schema. That's a much more involved "fix" that shouldn't even have to be coded given that, again, enum values are case insensitive in Adaptive.

The only reasonable solutions are:

adampauls commented 5 years ago

I agree that's the best solution. Any ideas as to a timeline? We will likely change a copy of the schema to do this for our own purposes, but hopefully that's a temporary state of affairs.

andrewleader commented 5 years ago

Hey @adampauls, I'm happy to report that we'll be fixing this with the 1.2 schema! Can you try it out here and see if the new schema passes validation for you?

New schema: http://adaptivecards-ci-typed.azurewebsites.net/schemas/1.2.0/adaptive-card.json