OData / ODataConnectedService

A Visual Studio extension for generating client code for OData Services
Other
79 stars 41 forks source link

Fix missing ContainerProperty attribute on dynamic properties (issue #380) #381

Closed stanb closed 5 months ago

stanb commented 6 months ago

Issues

This pull request fixes issue #380 When entity is an open type (<EntityType Name="ResultObject" OpenType="true">) VS extension generates IDictionary<string, object> DynamicProperties property. This property must have ContainerProperty attribute. Otherwise, when connected service is used, the dictionary is not populated from response. If in the project, referenced Microsoft.OData.Client nuget with version >= 7.10.*, ContainerProperty attribute not added due to wrong versions comparison. If I reference Microsoft.OData.Client version 7.20.0, the ContainerProperty attribute not added because lexically 7.20.0 < 7.6.4

Description

Fix Microsoft.OData.Client versions comparison.

stanb commented 6 months ago

@microsoft-github-policy-service agree

wachugamaina commented 6 months ago

Could you kindly share in the description what the issue is exactly and what the solution hopes to achieve?

stanb commented 6 months ago

@wachugamaina I hope this is enough.

habbes commented 5 months ago

@stanb thanks for this PR. Using lexical comparison was definitely a strange choice. Would you mind rebasing this PR with the master branch?

@gathogojr could you check why the pipeline is failing?

stanb commented 5 months ago

@habbes done

gathogojr commented 5 months ago

Hey @stanb. Your code change won't give you the outcome you expect. Microsoft.OData.Client version 7.6.4 didn't support the dynamic properties container property. Support was introduced in 7.7.* Take a look at the changelog here - https://learn.microsoft.com/en-us/odata/changelog/odatalib-7x#odatalib-770-beta-release It's the reason why the version was explicitly hard-coded. Your code would result into the dynamic properties container property being generated in the proxy but the logic to populate the dictionary with the materialized dynamic properties isn't available in 7.6.4 and lower versions of the library.

Change:

return Version.Parse(reference.Version) >= Version.Parse("7.6.4.0");

to

return Version.Parse(reference.Version) > Version.Parse("7.6.4.0");
gathogojr commented 5 months ago

@stanb thanks for this PR. Using lexical comparison was definitely a strange choice. Would you mind rebasing this PR with the master branch?

@gathogojr could you check why the pipeline is failing? A fix for the failures on the build pipeline had been merged.

@habbes The rebase is what was needed. It had

stanb commented 5 months ago

@gathogojr fixed