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.

Sign up for GitHub

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

Jump to bottom

fix issue with removeLinkStorages when specificaly querying the link field #388

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix issue with removeLinkStorages when specificaly querying the link field #388

wants to merge 7 commits into from

Conversation

vparpoil
Copy link
Contributor

This commit fixes this issue #386
Tests are still passing locally

@theodorDiaconu
Copy link
Contributor

I think the solution is in the graph preperation phase, not in the delivery.

@vparpoil
Copy link
Contributor Author

Can you indicate me which file/function you have in mind ?

I have been searching for the scheduledForDeletion param in the preparation of the graph but in the case referenced in the issue, the field authorId is never marked as scheduledForDeletion

As you can see in the code modified in this PR, the code removing the field is not testing for the scheduledForDeletion field anyway, but only removing collectionNode.linkStorageFields.
I don't see which part of the graph I should update in the preparation to avoid the issue (I am pretty sure that unsetting collectionNode.linkStorageFields is not an option here because it will break other logic)

@bhunjadi
Copy link
Contributor

bhunjadi commented Oct 22, 2019 •
edited
Loading

I think you should check createGraph.js for preparation.
There is also add function in collectionNode.js which sets conditionaly shouldCleanStorage.

It seems that order matters but it should not. _shouldCleanStorage depends on fields that were added and when authorId is after author field it does not catch it as added and therefore it is cleaned.

Solution could be to first separate db fields, linkers and reducers and add the db fields first and then other types of fields.

@vparpoil
Copy link
Contributor Author

Thanks for the insight !
indeed, _shouldCleanStorage is called after each linker node is added. Since the auhtorId has not been yet added to the CollectionNode, it is marked with the shouldCleanStorage attribute.
I will try to propose an alternative fix sorting root.body in the createNodes function

@vparpoil
Copy link
Contributor Author

@theodorDiaconu Here is an updated version modifying the body keys order to add the fields first to the CollectionNode. Let me know if you think it is not the good way to handle this.

theodorDiaconu
theodorDiaconu reviewed Nov 4, 2019
View reviewed changes
lib/query/lib/createGraph.js
@@ -82,6 +84,23 @@ export function createNodes(root) {
}
}

function sortFieldNames(body) {
const bodyCompareFunction = function (key, value) {
// push fields first
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if sorting is the right idea here. It may fail for nested links. I think the solution is to not mark the field as scheduled for deletion, or use it above the link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, with that sorting function we are doing kind of a guesswork. Why not make this process 2-pass? First, we determine what are props, fields, reducers and links. Second, we add them. Props and fields first, then links and reducers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your time reviewing
Since the fields are not yet added in _shouldCleanStorage, the hasField method cannot see that the field is supposed to be there.
We could update this line and change it with

result = !this.hasField(node.linkStorageField, true) && !(node.linkStorageField in node.parent.body)

Would that be a better solution @theodorDiaconu ?

@StorytellerCZ StorytellerCZ linked an issue Oct 6, 2022 that may be closed by this pull request
Fields asked in queries are not always in the response, depending on their order #386
Open
@StorytellerCZ
Copy link
Collaborator

@theodorDiaconu @vparpoil tests are now passing, so I want to inquire how you two feel about merging this.

@vparpoil
Copy link
Contributor Author

Hi @StorytellerCZ , I don't feel very confident myself because I was expecting some approval from @theodorDiaconu
I don't know the codebase enough and this was a long time ago. I could work on another proposition for this fix but I need some hints on how / where to fix the issue.
Thanks for bumping this PR !

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

@theodorDiaconu theodorDiaconu Awaiting requested review from theodorDiaconu

@bhunjadi bhunjadi Awaiting requested review from bhunjadi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fields asked in queries are not always in the response, depending on their order
4 participants
@vparpoil @theodorDiaconu @bhunjadi @StorytellerCZ