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

Cosmos: Fix reference nested owned entities in queries #16276

Merged
merged 2 commits into from Jun 27, 2019
Merged

Conversation

AndriySvyryd
Copy link
Member

No description provided.

@AndriySvyryd
Copy link
Member Author

@smitpatel Could you check whether this is the correct approach?

_materializationContextBindings[parameterExpression] = Expression.Convert(
CreateReadJTokenExpression(_jObjectParameter, projection.Alias),
typeof(JObject));
_materializationContextBindings[parameterExpression] = _jObjectParameter;
Copy link
Member

Choose a reason for hiding this comment

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

Does this work when you have projected out multiple entities in projection

Copy link
Member Author

Choose a reason for hiding this comment

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

It should. Is there a test for that?

shaperBody = new CosmosProjectionBindingRemovingExpressionVisitor(selectExpression, jObjectParameter)
.Visit(shaperBody);
var jObjectParameter = Expression.Parameter(typeof(JObject), "jObject");
var shaperBody = new CosmosProjectionBindingRemovingExpressionVisitor(selectExpression, jObjectParameter, this)
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep injecting materializer and binding removal separate?
We can have another visitor which inject nested entityshapers

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my original design, but we need to add the jObject variable before each materializer is injected.

@AndriySvyryd AndriySvyryd marked this pull request as ready for review June 26, 2019 21:24
@AndriySvyryd AndriySvyryd changed the title Cosmos Query: Fix nested owned entities in queries Cosmos: Fix reference nested owned entities in queries Jun 26, 2019
@AndriySvyryd
Copy link
Member Author

@smitpatel Updated

@smitpatel
Copy link
Member

It would be good to have EntityShaper not worry about nested part and at some point before compiling, we injected nested entityShaper in the tree directly. (possibly re-using includeExpression),
That gives us pure entity shaper. Injecting nested shapers/injecting materializers/removing bindings as separate phases.
If there is anything blocking or difficult in that, we can defer for now.
Otherwise PR looks good. :shipit:

@AndriySvyryd
Copy link
Member Author

Ok, I'll check this in as is and try that while adding collection support.

@AndriySvyryd AndriySvyryd merged commit 95c1c54 into master Jun 27, 2019
@ghost ghost deleted the CosmosOwned branch June 27, 2019 16:48
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.

None yet

2 participants