Fixes #29#31
Draft
blitzmann wants to merge 5 commits into
Draft
Conversation
Here we identify the node that filters on something like posts/id, and sets the relation in the included so that data can be pulled back without expand. There is currently a bug tho - if the filter comes before the expand, the expand doesn't set the selections correctly. This is due to us already having visited the table with the filter, and setting select = '' (hardcoded)
(cherry picked from commit 0ef5c0a)
…d filter inclusions for select alias processing) * Remove `expands` property - Not entirely sure the purpose of this, looks like it was supposed to work similar in that it was storing selections that needed to be expanded first, and then adding them during the expansion * Fixed bug where a `,` was being missed while adding to `this.select` (cherry picked from commit 78f4c5d)
(cherry picked from commit 7f29553) # Conflicts: # examples/server/src/migrations/1577087002356-dataFilling.ts # examples/server/src/server.ts
Contributor
Author
|
@andryuha49 I added what I hope are simple testing instructions. :) |
Contributor
Author
|
@andryuha49 when do we think we can review this PR? This one is actively blocking some of our development, so want to make the review as smooth as possible, anything I can do to help? |
Merged
Contributor
Author
|
@andryuha49 I believe I found an issue with this PR in my other work. While the alias is being set correctly, if you filter on that relationship, then the filter does not reflect that new alias, and instead it is shown as the original name. I don't have a good test / example of this at the moment, but I'll try to dig into soon. I'm going to change this PR to a draft in the meantime |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #29, Requires #28 as I used it's sorted evaluation.
Essentially, this changes the way alias's work. We no longer assume the alias = the navigation property, because in that scenerio you can't join two two or more of the same table. This PR changes the alias to be the property, plus the current position of the token, which should make them all unique since tokens can't share the same position, and it also gives helpful debugging information right there in the alias name.
Also fixes an issue with having a
,missed.Unfortunately, I don't really have a good test of this at the moment. I used my own test entities, but it doesn't make sense in this repo. I will try to add actual test cases to the example server.
Additionally, it's getting harder to split these changes up into PR's from my other improvements. I would love to continue to contribute, but if this project is no longer maintained, then please mention that on the README and I won't have to make the effort. But as it stands now, I might not be able to continue to move forward with additional PRs without them stepping on one another
Testing
d27b91from this PR. This will give you new entities configured in a way that will show this problemThe new entities are: User, and PostComment. A post has several comments, and each comment is associated with a User. Additionally, the Author now is associated with a User.
Pre-fix
Start the example server with the new entities and migration, and then run this:
{{base_url}}/api/posts?$select=id,title&$expand=author($expand=user),comments($expand=user)&$filter=id eq 1This is because we're trying to expand a
userproperty twice - once inauthor, and once incomments. Since we use the property path as the alias, there's two joins with the alias ofuserPost-fix
We can now bring back user relations for both author and all the comments. This is because we change the alias that is used to be unique to that specific expansion
{{base_url}}/api/posts?$select=id,title&$expand=author($expand=user),comments($expand=user)&$filter=id eq 1