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

Fix relation/indexing processing Data object collision #723

Open
wants to merge 3 commits into
base: rel6
Choose a base branch
from

Conversation

c58
Copy link

@c58 c58 commented May 26, 2024

processRelated and doIndex may be executed for the same operation object, doIndex mutates the Data of the operation which breaks the relation processing if the relation processing happens after the indexing.

The effect of such a collision may be observed as printed errors like "Source field _id not found in document: ...", because doIndex removes the _id field from the Data object as part of the prepareDataForIndexing function but extractData attempts to read it in case if the src-field="_id"

The fix separates copies of Data between relation processing and indexing, which eliminates the possibility of such a collision.

processRelated and doIndex may be executed for the same operation object, doIndex mutates the Data of the operation which breaks the relation processing if it happens after the indexing which produces errors "Source field %s not found in document". The fix separates copies of Data between relation processing and indexing
@c58 c58 changed the title Fix relation/indexing Data object collision Fix relation/indexing processing Data object collision May 26, 2024
@@ -1336,7 +1336,29 @@ func (ic *indexClient) processRelated(root *gtm.Op) (err error) {
}
}
if visit {
q = append(q, rop)
Copy link
Author

@c58 c58 May 26, 2024

Choose a reason for hiding this comment

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

rop is queued here for further relations processing, and also the same rop is added to the indexC here, so both may mutate the same Data

@c58
Copy link
Author

c58 commented May 27, 2024

@rwynn first of all, thank you for the amazing tool!

I hope you can find some time to check my PR. For now i switched my dockerfiles to pull monstache from my forked repo, so no rush on checking it, however I think the bug that I faced may affect some other people who have many [[relate]] in the config and the current version of monstahce may silently work unexpectedly since the printed error doesn't crash the monstache instance.

Please let me know if you would like me to provide more context or somehow adjust the PR, I'm ready to do that.

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.

1 participant