Skip to content

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

Merged
habbes merged 4 commits intoOData:masterfrom
stanb:stanb/fix-380
Mar 26, 2024
Merged

Fix missing ContainerProperty attribute on dynamic properties (issue #380)#381
habbes merged 4 commits intoOData:masterfrom
stanb:stanb/fix-380

Conversation

@stanb
Copy link
Copy Markdown
Contributor

@stanb stanb commented Mar 5, 2024

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
Copy link
Copy Markdown
Contributor Author

stanb commented Mar 5, 2024

@microsoft-github-policy-service agree

@wachugamaina
Copy link
Copy Markdown

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

@stanb
Copy link
Copy Markdown
Contributor Author

stanb commented Mar 8, 2024

@wachugamaina I hope this is enough.

@habbes
Copy link
Copy Markdown
Contributor

habbes commented Mar 22, 2024

@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
Copy link
Copy Markdown
Contributor Author

stanb commented Mar 23, 2024

@habbes done

@gathogojr
Copy link
Copy Markdown
Contributor

gathogojr commented Mar 23, 2024

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
Copy link
Copy Markdown
Contributor

@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
Copy link
Copy Markdown
Contributor Author

stanb commented Mar 23, 2024

@gathogojr fixed

if (reference.Name.Equals("Microsoft.OData.Client", StringComparison.Ordinal))
{
return true;
return Version.Parse(reference.Version) > Version.Parse("7.6.4.0");
Copy link
Copy Markdown
Contributor

@ElizabethOkerio ElizabethOkerio Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a parsing exception here? or reference.Version will always be in the required format?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially because the reference is for a library we maintain, plus this statement is in within the block

if (reference.Name.Equals("Microsoft.OData.Client", StringComparison.Ordinal))

I think this should be safe.
In addition, we currently update the Microsoft.OData.Client reference manually.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants