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

Add ReplaceIllegalFieldNameCharactersAttribute, add related tests, fixes issue #1269 #1273

Open
wants to merge 3 commits into
base: release-8.x
Choose a base branch
from

Conversation

marcus905
Copy link

As previously mentioned in #1269 this adds the attribute in the pull request title to replace illegal characters in field names with legal ones before serialization begins.

@marcus905
Copy link
Author

@microsoft-github-policy-service agree

namespace Microsoft.AspNetCore.OData.Formatter.Attributes
{
[AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)]
sealed class ReplaceIllegalFieldNameCharactersAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use an attribute to change the property name looks weird to me. Since, it requires customers to change their data model by decorating this attribute. Most of time, it's not acceptable.

I think we can have a different option:

  1. Create a service (an interface) named 'IEscapeCharacterService' or similar
  2. Consume this service during 'AppendDynamicProperties'
  3. If we have this service registered, calling it. Otherwise, let it go and no other impact for all existing customers

Where to register this service?

  1. We can let it register into the DI, or
  2. We can let it register into the Edm Model?

Copy link
Author

@marcus905 marcus905 Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the attribute to avoid introducing any breaking change like automatic renaming for everyone (breaking the code for users manually disabling the validator, as seen in issue #1423 of OData.net OData/odata.net#1423 for example) and to provide an avenue to allow choosing the new replacement strings for each of the invalid characters.

Moreover, having the user code a service that implements that interface himself if they need it when it is actually very straightforward to implement it easily, verifiably and correctly for the scope of the fix (which is not a facility to let people manipulate the property names, but merely a choice between fail-if-illegal-char-in-property-name and rename-property-and-serialize, with just a hint of customization on replacements @ may become _, or x0040 or anything the user wants that is acceptable) would be to burden them with another fair bit of avoidable boilerplate code.

If the user is already using the lib successfully, then there will be no need to apply that attribute and the renaming would not happen at all (no regressions on other tests, assert exception passed on the invalid mapped dynamic property dictionary) and so customers/user would be safe as no downstream code changes will be needed for normal operations.

If the user, instead, needs to fix the property names (and mind you, even if I do not foresee it being ever used in this way) this way, by annotating the model class, it would work on both dynamic and static properties which have a specific name set all the same.

I concede that the naming of the attribute might not be the best, but that can be changed.

Moreover, annotating entities in this way is a Model pattern classic (EF for example, but it's also used in several other different libraries in other frameworks and programming languages.)

I still consider this to be the best approach, but if this is deemed to not be acceptable for the project, I'll try yours.

Please let me know.

@marcus905
Copy link
Author

After some testing, the '#' char has a special meaning for most OData clients too (Power BI for example), so I added a way to replace it in the code.

I'm waiting for your review, thanks.

@marcus905
Copy link
Author

Is there any news on this?

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

Successfully merging this pull request may close these issues.

2 participants