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

Liquid templates ContentItems property names became case insensitive #16016

Open
SzymonSel opened this issue May 8, 2024 · 12 comments
Open

Liquid templates ContentItems property names became case insensitive #16016

SzymonSel opened this issue May 8, 2024 · 12 comments
Labels
Milestone

Comments

@SzymonSel
Copy link
Member

SzymonSel commented May 8, 2024

After upgrading from 1.9 to 2.0 we've noticed some of our liquid templates failing with a the error

ArgumentException: An item with the same key has already been added. Key: Camelcase (Parameter 'propertyName')

This happend because we are calling {{ field.MyProperty.CamelCase.Text }}

This just so happens that in some time during design and development we misstyped the name of the field and later changed it from Camelcase to CamelCase which left us with some harmless rubish in the JSON content... well at least until we upgraded to OC 2.0

{"CamelCase":{"Text":null},"Camelcase":{"Text":null}}

Is this switch, to case-insensitiveness was done intentionally?

@Piedone Piedone added this to the 2.0 milestone May 8, 2024
@Piedone
Copy link
Member

Piedone commented May 8, 2024

@MikeAlhayek can you comment on this, please?

@hishamco
Copy link
Member

hishamco commented May 8, 2024

Might the naming policy have been change after introducing STJ

CamelCase = new JsonSerializerOptions(Default)
{
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
};

@hishamco
Copy link
Member

hishamco commented May 8, 2024

I just notice

@MikeAlhayek it's on your plate while you complete the JT PR

@Piedone
Copy link
Member

Piedone commented May 8, 2024

That's not something we want to change, though. It seems that while the previous de/serialization logic was more forgiving, in the end, the code had a bug that's now surfaced. That's good. We may make the exception more user-friendly though if feasible.

@SzymonSel
Copy link
Member Author

What's the bug in particular?

It is true, that previous de/serialization logic was more forgiving, but the CaseInsesitivity is a mystery for me. I can't see it's purpose.

How do we ensure, this doesn't happen again? The user interface shouldn't allow this.

@Piedone
Copy link
Member

Piedone commented May 9, 2024

The bug is in your code :). I don't think there is one in OC. Identifiers everywhere in C# and OC are case-sensitive apart from selected cases, so I don't see why we would try to allow case insensitivity for part or field names (which are case-sensitive anyway) in Liquid. (To clarify, what this issue is about is case-sensitivity, not case-insensitivity, since your code worked because previously the serializer was case-insensitive.)

@MikeAlhayek
Copy link
Member

@hishamco I don't think PropertyNameCaseInsensitive = true, has anything to do with this.

When the PropertyNameCaseInsensitive property is set to true during deserializing, it allows you to map a property from any form back into the object.

For example, if you have this class

public class Person
{
    public string FirstName { get; set; }	
}

you can deserialize this with no problem.

{"FirstName":"Mike"}

But, the following will only be deserialize correctly if PropertyNameCaseInsensitive = true.

{"firstName":"Mike"}

I am not very familiar Fluid project which may be the cause of the issue. @sebastienros

@SzymonSel
Copy link
Member Author

The bug is in your code :). I don't think there is one in OC. Identifiers everywhere in C# and OC are case-sensitive apart from selected cases, so I don't see why we would try to allow case insensitivity for part or field names (which are case-sensitive anyway) in Liquid. (To clarify, what this issue is about is case-sensitivity, not case-insensitivity, since your code worked because previously the serializer was case-insensitive.)

  • No, you understood me wrong. It's the other way around. I want the indentifiers to be case sensitive, but they aren't anymore in Liquid Templates. Hence after an update to OC 2.0, we started having the above problem.

We created a field named "Camelcase", then we created some contentItems, the we removed the field "Camelcase" and created a new one named "CamelCase". Everything worked fine until we upgraded to OC 2.0

@Piedone
Copy link
Member

Piedone commented May 9, 2024

Ah OK, got it, sorry about the confusion.

@sebastienros
Copy link
Member

ArgumentException: An item with the same key has already been added. Key: Camelcase (Parameter 'propertyName')

What is the full stack trace to understand which component is not happy?

@sebastienros
Copy link
Member

This could be from

options.MemberAccessStrategy.Register<JsonObject, object>((source, name) => source[name]);

Where in JSON.NET the accessor would be case insensitive, but STJ might be case-sensitive.

@sebastienros
Copy link
Member

They behave the same, case sensitive https://dotnetfiddle.net/CxK4ls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants