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

Issue with data conversion errors. #2906

Open
johnwc opened this issue Mar 21, 2024 · 5 comments
Open

Issue with data conversion errors. #2906

johnwc opened this issue Mar 21, 2024 · 5 comments

Comments

@johnwc
Copy link

johnwc commented Mar 21, 2024

We are pulling from a third party oData provider that does not adhere to their schema. They are a middleman that takes in data from their clients and turns it into a oData service on the client's behalf. Instead of validating the data against the outgoing schema, they just allow data to flow. If the schema states a property is supposed to be a Edm.Int32, they will allow a value of 2.5 to pass through. Which causes a ODataExceptopn within ConvertFromPayloadValue

Assemblies affected

7.20.0

Reproduce steps

Internal

Expected result

Error to include the name of the property with the invalid data.

Actual result

Generic exception stating Cannot convert the literal '2.5' to the expected type 'Edm.Int32'.

Additional detail

Will you accept a PR with an update to ODataPayloadValueConverter to add a parameter to ConvertFromPayloadValue to pass the name of the property being converted, so that it can be included in the ODataException? I would add an overload for the existing method, so to keep backwards compatibility to anything using ConvertFromPayloadValue without the property name.

/// <summary>
/// Converts the given payload value to the type defined in a type definition.
/// </summary>
/// <param name="value">The given payload value.</param>
/// <param name="edmTypeReference">The expected type reference from model.</param>
/// <returns>The converted value of the type.</returns>
public virtual object ConvertFromPayloadValue(object value, IEdmTypeReference edmTypeReference)
    => ConvertFromPayloadValue(value, edmTypeReference, null);

/// <summary>
/// Converts the given payload value to the type defined in a type definition.
/// </summary>
/// <param name="value">The given payload value.</param>
/// <param name="edmTypeReference">The expected type reference from model.</param>
/// <returns>The converted value of the type.</returns>
public virtual object ConvertFromPayloadValue(object value, IEdmTypeReference edmTypeReference, string? propertyName)
{
     ...
}

// Making a similar update to GetPrimitiveTypeConversionException to accept the propertyName
@gathogojr
Copy link
Contributor

Hi @johnwc. This is not an issue that should require a change to the library since it's not a bug in the implementation or a scenario that cuts across users of the library. Our advice is for you to create a custom ODataPayloadValueConverter that implements the behaviour you want and inject it into the DI container. See similar advise provided for a related case - #2464 (comment)

@johnwc
Copy link
Author

johnwc commented Mar 26, 2024

@gathogojr i thought of doing that first, until I noticed there was no way to know what the details of the property is from that method. It only gets passed the raw value and its expected type. How do you suggest I throw an error inside my custom class with the name of the property?

Also, a lot of times things are not done because it only effects the library, sometimes things are done to help the developers and end users of the library. This is a perfect example of such that, it helps the end user know what is wrong and where it is.

@gathogojr
Copy link
Contributor

@johnwc I understand. We triaged this issue earlier today and came to the same conclusion as you have. A custom payload value converter won't make it possible to report the property that has an invalid value.
We'd be open to a pull request contribution if you're in a position to do that. Luckily we can introduce the change in ODataPayloadValueConverter with minimal/no risk of breaking other customers.

@johnwc
Copy link
Author

johnwc commented Mar 26, 2024

@gathogojr Are you good with the changes I documented above?

@gathogojr
Copy link
Contributor

The suggestion looks okay. If you can publish a pull request then we can review. Add tests to verify the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants