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

Grouping by a nested dynamic property differs from grouping by a nested standard property #2963

Closed
clemvnt opened this issue May 7, 2024 · 3 comments
Labels

Comments

@clemvnt
Copy link
Contributor

clemvnt commented May 7, 2024

Hello,

I have a case where I want to group by a nested dynamic property.

The result is not the same as when I group by a nested standard property.

Is there an explanation for this or is it a bug?

Assemblies affected

Microsoft.AspNetCore.OData 8.2.5

Reproduce steps

  1. Clone the repository : https://github.com/clemvnt/ODataGroupByNestedDynamicProperty
  2. Run

Actual result

  1. /customers => OK
  2. /customers?$apply=groupby((Address/City)) => OK
[
    { "Address": { "City": "City 2" } },
    { "Address": { "City": "City 1" } }
]
  1. /customers?$apply=groupby((Address/DynamicCity)) => NOK
[
    { "DynamicCity": "City 2" },
    { "DynamicCity": "City 1" }
]

Expected result

I would expect the following result for the third query :

[
    { "Address": { "DynamicCity": "City 2" } },
    { "Address": { "DynamicCity": "City 1" } }
]
@clemvnt clemvnt added the bug label May 7, 2024
@clemvnt
Copy link
Contributor Author

clemvnt commented May 8, 2024

By making the following changes on the Microsoft.OData.Core side, it works:
clemvnt@bb6047b

Several questions:

  • Does that sound good to you? If so, I'll add the tests and make a PR.
  • That seems like a breaking change to me, no?
  • Since this concerns the Microsoft.OData.Core project, is it possible to move the issue to the odata.net repository?

@xuzhg
Copy link
Member

xuzhg commented May 9, 2024

@clemvnt Thanks for reporting this. Would you please share a PR for us to review it?

@gathogojr gathogojr transferred this issue from OData/AspNetCoreOData May 14, 2024
@gathogojr
Copy link
Contributor

@clemvnt Thank you for reporting this issue and for the thorough investigation. To answer your questions:

  • The fix looks good. Great work!
  • From a behaviour perspective, this would be considered a breaking change. However, we'd be ready to make a concession here and allow this change since what we're writing to the wire is actually invalid. I'd hate to imagine that anyone is taking a dependency on this invalid payload
  • We have transferred the issue to the right repo

Kindly create a pull request and add some tests. We'll be happy to review.
Thanks

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

No branches or pull requests

3 participants