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

YamlMember Alias isn't applied when using the CamelCaseNamingConvention #228

Closed
ghost opened this issue Dec 15, 2016 · 11 comments
Closed

YamlMember Alias isn't applied when using the CamelCaseNamingConvention #228

ghost opened this issue Dec 15, 2016 · 11 comments

Comments

@ghost
Copy link

ghost commented Dec 15, 2016

If I ask a deserializer to use the CamelCaseNamingConvention then it appears to ignore or otherwise fail to use an Alias declared using a YamlMember attribute.

What I Expected to Happen

The CamelCaseNamingConvention should be applied to all property names.
The YamlMember Alias should then override the property names where it exists.

What Actually Happened

I get an exception saying the property can't be found despite using the YamlMember Alias.

Exception

Run-time exception (line 27): (Line: 3, Col: 5, Idx: 8) - (Line: 3, Col: 5, Idx: 8): Exception during deserialization

Stack Trace:

[System.Runtime.Serialization.SerializationException: Property 'sub_item' not found on type 'Item'.]

[YamlDotNet.Core.YamlException: (Line: 3, Col: 5, Idx: 8) - (Line: 3, Col: 5, Idx: 8): Exception during deserialization]
  at Program.Main(): line 27

Reproduction/Code

Here's a reproduction of the issue:

https://dotnetfiddle.net/ZOmLwf

using System.Collections.Generic;
using System.IO;
using YamlDotNet.Serialization;
using YamlDotNet.Serialization.NamingConventions;
					
public class Program
{
	public static void Main()
	{
		var yaml = @"
1:
    typeID: 1
    sub_item:
        name: foo
2:
    typeID: 2
    sub_item:
        name: bar
";
		
		var deserializer = new DeserializerBuilder()
			.WithNamingConvention(new CamelCaseNamingConvention())
			.Build();
		
		using (var textReader = new StringReader(yaml))
		{
			var blueprintsByID = deserializer.Deserialize<Dictionary<int, Item>>(textReader);
		}
	}
}

public class Item
{
	public int TypeID { get; set; }
	
	[YamlMember(Alias ="sub_item")]
	public SubItem SubItem { get; set; }
}

public class SubItem
{
	public string Name { get; set; }
}

And here's an example without the CamelCaseNamingConvention where it's working fine:

https://dotnetfiddle.net/pwbw7R

using System.Collections.Generic;
using System.IO;
using YamlDotNet.Serialization;
					
public class Program
{
	public static void Main()
	{
		var yaml = @"
1:
    typeID: 1
    sub_item:
        name: foo
2:
    typeID: 2
    sub_item:
        name: bar
";
		
		var deserializer = new DeserializerBuilder()
			.Build();
		
		using (var textReader = new StringReader(yaml))
		{
			var blueprintsByID = deserializer.Deserialize<Dictionary<int, Item>>(textReader);
		}
	}
}

public class Item
{
	[YamlMember(Alias ="typeID")]
	public int TypeID { get; set; }
	
	[YamlMember(Alias ="sub_item")]
	public SubItem SubItem { get; set; }
}

public class SubItem
{
	[YamlMember(Alias ="name")]
	public string Name { get; set; }
}

Workaround

And here's a workaround I found in issue: #215

https://dotnetfiddle.net/RL9Eai

using System.Collections.Generic;
using System.IO;
using YamlDotNet.Serialization;
using YamlDotNet.Serialization.NamingConventions;
using YamlDotNet.Serialization.TypeInspectors;
					
public class Program
{
	public static void Main()
	{
		var yaml = @"
1:
    typeID: 1
    sub_item:
        name: foo
2:
    typeID: 2
    sub_item:
        name: bar
";
		
		var deserializer = new DeserializerBuilder()
			.WithNamingConvention(new CamelCaseNamingConvention())
			// Workaround to move YamlAttributesTypeInspector
			.WithTypeInspector
			(
				inner => inner,
				s => s.InsteadOf<YamlAttributesTypeInspector>()
			)
			.WithTypeInspector
			(
				inner => new YamlAttributesTypeInspector(inner),
				s => s.Before<NamingConventionTypeInspector>()
			)
			.Build();
		
		using (var textReader = new StringReader(yaml))
		{
			var blueprintsByID = deserializer.Deserialize<Dictionary<int, Item>>(textReader);
		}
	}
}

public class Item
{
	public int TypeID { get; set; }
	
	[YamlMember(Alias ="sub_item")]
	public SubItem SubItem { get; set; }
}

public class SubItem
{
	public string Name { get; set; }
}
@ghost
Copy link
Author

ghost commented Dec 15, 2016

It appears this is a result of the changes made during: #190

@ghost
Copy link
Author

ghost commented Dec 15, 2016

More information is available in ticket: #215

@ghost
Copy link
Author

ghost commented Dec 15, 2016

The workaround mentioned in #215 works for me.

I still think this behaviour is a bug though as it's unexpected.

If an Alias is provided then it should be used unmodified, as you're providing the exact name that should be used.

@aaubry
Copy link
Owner

aaubry commented Dec 15, 2016

This behavior is not consensual. @veleek also has a valid argument for applying the naming convention after the alias. Maybe there should be a method on the builder to choose between the two behaviors?

@leepa
Copy link

leepa commented Jan 8, 2017

Sure but the example on the main website doesn't work as a result.

@aaubry
Copy link
Owner

aaubry commented Jan 10, 2017

You are right about the examples being broken. It has already been reported in #230, and this will need to be fixed. If you want to participate in that discussion, please comment on that thread. Thanks

@ghost
Copy link
Author

ghost commented Jan 12, 2017

A method on the builder to switch modes for the entire context seems reasonable.

Another option is adding a property to the YamlMember attribute that controls it for this specific member.

[YamlMember(Alias ="typeID", ApplyNamingConventions = false)]

I quite like that as it means you could also disable the naming conventions on a member that doesn't have an alias.

The "real" solution in my case is to go slap the data provider for using a single name which doesn't conform to the naming standards they use elsewhere. Then I wouldn't need aliases at all! But at this point it's "legacy code" that's hard for them to change.

@aaubry
Copy link
Owner

aaubry commented Jan 18, 2017

Maybe the suggestion of having a ApplyNamingConventions property on YamlMember is the best compromise that we can reach. If we go for it, what should the default value be? true to preserve the current behavior or false to revert to the pre-4.0.0 behavior?

@ghost
Copy link
Author

ghost commented Jan 18, 2017

To recap (mainly to keep things straight in my mind):

What we want is a way to turn off the NamingConventions when Alias is being used.

The proposed approach would be adding a property called ApplyNamingConventions to the YamlMember attribute which can turn off the NamingConventions independently of the Alias property.

I can see three options for what he default can be:

  • true
    • 'NamingConventions' are applied to all members by default.
  • false
    • 'NamingConventions' are applied to no members by default.
  • == !string.IsNullOrWhiteSpace(alias)
    • 'NamingConventions' are not applied to members with an alias by default.

The second option would globally disable NamingConventions and that feels wrong.
A convention is "a way in which something is usually done" so it should be global! What we want is to opt out under special circumstances.

The third option does work, but is overly complicated making it more fragile and possibly confusing to developers using the library.

Finally, there's the first option. With that approach the NamingConventions work as expected, but we can explicitly opt out of them for problematic members. (With the added bonus of not having to make a breaking change to the 4.0.0+ version).

@aaubry
Copy link
Owner

aaubry commented Jan 18, 2017

Thanks for this recap. I am now convinced that defaulting to true seems to be the best option. This also avoids another breaking change.

@aaubry aaubry mentioned this issue Jan 18, 2017
@aaubry
Copy link
Owner

aaubry commented Jan 18, 2017

I added the ApplyNamingConventions property to YamlMemberAttribute, with the behavior defined here. I also added the samples to a new test project and made the necessary changes for them to pass.

@aaubry aaubry closed this as completed Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants