odata.net icon indicating copy to clipboard operation
odata.net copied to clipboard

Issue with data conversion errors.

Open johnwc opened this issue 11 months ago • 5 comments

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

johnwc avatar Mar 21 '24 16:03 johnwc

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 - https://github.com/OData/odata.net/issues/2464#issuecomment-1734333370

gathogojr avatar Mar 26 '24 14:03 gathogojr

@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.

johnwc avatar Mar 26 '24 17:03 johnwc

@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.

gathogojr avatar Mar 26 '24 19:03 gathogojr

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

johnwc avatar Mar 26 '24 19:03 johnwc

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

gathogojr avatar Mar 26 '24 19:03 gathogojr