-
Notifications
You must be signed in to change notification settings - Fork 161
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
marcus905
wants to merge
3
commits into
OData:release-8.x
Choose a base branch
from
marcus905:main
base: release-8.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
63 changes: 63 additions & 0 deletions
63
...osoft.AspNetCore.OData/Formatter/Attributes/ReplaceIllegalFieldNameCharactersAttribute.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
using System; | ||
using System.Linq; | ||
|
||
namespace Microsoft.AspNetCore.OData.Formatter.Attributes | ||
{ | ||
[AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)] | ||
sealed class ReplaceIllegalFieldNameCharactersAttribute : Attribute | ||
{ | ||
//constant collection of illegal characters | ||
private static readonly string[] illegalChars = new string[] { "@", ":", ".", "#" }; | ||
public string ReplaceAt { get; } | ||
public string ReplaceColon { get; } | ||
public string ReplaceDot { get; } | ||
public string ReplaceHash { get; } | ||
|
||
public ReplaceIllegalFieldNameCharactersAttribute(string replaceAt, string replaceColon, string replaceDot, string replaceHash) | ||
{ | ||
//check if the replacement characters are not null | ||
if (replaceAt == null || replaceColon == null || replaceDot == null) | ||
{ | ||
throw new ArgumentNullException("Replacement characters cannot be null"); | ||
} | ||
|
||
// check if any of the the replacement characters provided contain any of the illegal characters | ||
// ex. if replaceAt contains any of the illegal characters checked one by one | ||
if (illegalChars.Any(illegalChar => replaceAt.Contains(illegalChar) || replaceColon.Contains(illegalChar) || replaceDot.Contains(illegalChar))) | ||
{ | ||
throw new ArgumentException("Replacement character cannot be an illegal character"); | ||
} | ||
|
||
ReplaceAt = replaceAt; | ||
ReplaceColon = replaceColon; | ||
ReplaceDot = replaceDot; | ||
ReplaceHash = replaceHash; | ||
} | ||
|
||
public ReplaceIllegalFieldNameCharactersAttribute(string replaceAnyIllegal) | ||
{ | ||
if (illegalChars.Any(illegalChar => replaceAnyIllegal.Contains(illegalChar))) | ||
{ | ||
throw new ArgumentException("Replacement character cannot be an illegal character"); | ||
} | ||
|
||
ReplaceAt = replaceAnyIllegal; | ||
ReplaceColon = replaceAnyIllegal; | ||
ReplaceDot = replaceAnyIllegal; | ||
ReplaceHash = replaceAnyIllegal; | ||
} | ||
|
||
public ReplaceIllegalFieldNameCharactersAttribute() | ||
{ | ||
ReplaceAt = "_"; | ||
ReplaceColon = "_"; | ||
ReplaceDot = "_"; | ||
ReplaceHash = "_"; | ||
} | ||
|
||
public string Replace(string fieldName) | ||
{ | ||
return fieldName.Replace("@", ReplaceAt).Replace(":", ReplaceColon).Replace(".", ReplaceDot).Replace("#",ReplaceHash); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
Where to register this service?
There was a problem hiding this comment.
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.