-
Notifications
You must be signed in to change notification settings - Fork 351
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
Implement int to enum deserialization. #1590
base: release-7.x
Are you sure you want to change the base?
Implement int to enum deserialization. #1590
Conversation
Hey, I posted an issue about enum deserialization, where the read value is not checked against the metadata (#1593). I was about to make a PR about it, but i saw this PR, which is touching the same method. It would be easier to merge if the fix is applied as part of this PR, if everybody is ok with it of course. @bdebaere what do you think about adding something like: if (selectedMember == null && this.MessageReaderSettings.ThrowIfTypeConflictsWithMetadata)
{
throw new ODataException(Strings.ValidationUtils_EnumMemberDoesNotExistOnType(enumValue, propertyName, edmEnumType.FullTypeName()));
} and you must always try to get the or we can extend ReaderValidationUtils with a similar to
Thanks! |
@adzhilyanov I'd be happy to merge, I haven't gotten around to it yet. |
@xuzhg , @KanishManuja-MS could we get your review on this? |
return new ODataEnumValue(enumStr, expectedValueTypeReference.FullName()); | ||
object enumValue = this.JsonReader.ReadPrimitiveValue(); | ||
string enumString; | ||
if (enumValue is int enumIntValue) |
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.
enumValue is int enumIntValue [](start = 16, length = 29)
This new syntax maybe not pass .NET 4.5 and .NET 3.5 compiler. Please change it to normal.
else | ||
{ | ||
throw new ODataException(ODataErrorStrings.ReaderValidationUtils_CannotConvertPrimitiveValue(enumValue, typeof(IEdmEnumTypeReference))); | ||
} |
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'd like to see Unit test to cover the changes.
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.
🕐
When will be done this issue ? |
@bdebaere Kindly work on review comments. |
{ | ||
IEdmEnumType edmEnumType = expectedValueTypeReference.EnumDefinition(); | ||
IEdmEnumMember selectedMember = edmEnumType.Members.FirstOrDefault(member => member.Value.Value == enumIntValue); | ||
enumString = selectedMember.Name; |
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.
Can't selectedMember
be null
here because of FirstOrDefault
potentially not finding an element?
Issues
This pull request fixes issue OData/WebApi#1902.
Description
Reads a PrimitiveValue instead of a StringValue and tries to convert the value to an enum.
Checklist (Uncheck if it is not completed)