search

OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.23k stars 2.34k forks source link

Liquid templates ContentItems property names became case insensitive #16016

Open SzymonSel opened 2 months ago

SzymonSel commented 2 months ago

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 commented 2 months ago

@MikeAlhayek can you comment on this, please?

hishamco commented 2 months ago

Might the naming policy have been change after introducing STJ

https://github.com/OrchardCMS/OrchardCore/blob/23ee4f18f6dfda40fa366f58ce2559e270d23b55/src/OrchardCore/OrchardCore.Abstractions/Json/JOptions.cs#L44-L47

hishamco commented 2 months ago

I just notice

https://github.com/OrchardCMS/OrchardCore/blob/23ee4f18f6dfda40fa366f58ce2559e270d23b55/src/OrchardCore/OrchardCore.Abstractions/Json/JOptions.cs#L19

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

Piedone commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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.)

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 commented 2 months ago

Ah OK, got it, sorry about the confusion.

sebastienros commented 1 month ago

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 commented 1 month ago

This could be from https://github.com/OrchardCMS/OrchardCore/blob/9a8aa1c4644a047e67ac27bd2905dfde0326e372/src/OrchardCore.Modules/OrchardCore.Liquid/Startup.cs#L51

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

sebastienros commented 1 month ago

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

MikeAlhayek commented 1 month ago

@SzymonSel can you please address the last comments from @sebastienros.

SzymonSel commented 1 month ago

Sure, I'll be back shortly with a reply!

sebastienros commented 1 month ago

Having the conflicting property names and case insensitivity support works fine with STJ, so this shouldn't be the problem, we'll wait for the stacktrace to understand where it's coming from:

Example

sebastienros commented 1 month ago

If possible see the liquid template that failed with such data. To get the data, create a TextField then another one with the similar name (might need to remove the previous one, the data will be kept in the json document)

github-actions[bot] commented 1 month ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.