Skip to content

Prevent yield from operating on nodes which did not come from the current view. (SYN-5679) #3218

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

Merged
merged 5 commits into from
Jul 6, 2023

Conversation

vEpiphyte
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.11 ⚠️

Comparison is base (b43baff) 97.31% compared to head (a04dcf4) 97.20%.

❗ Current head a04dcf4 differs from pull request most recent head 46c4298. Consider uploading reports for the commit 46c4298 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3218      +/-   ##
==========================================
- Coverage   97.31%   97.20%   -0.11%     
==========================================
  Files         225      225              
  Lines       44998    45005       +7     
==========================================
- Hits        43788    43747      -41     
- Misses       1210     1258      +48     
Flag Coverage Δ
linux 97.20% <100.00%> (-0.01%) ⬇️
linux_replay ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
synapse/lib/ast.py 98.93% <100.00%> (+<0.01%) ⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vEpiphyte vEpiphyte added the reqChangelog requires changelog label Jul 6, 2023
Copy link
Contributor

@Cisphyx Cisphyx left a comment

Choose a reason for hiding this comment

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

I think we might need to check the view in the if isinstance(valu, s_stormtypes.Prim): case as well, I think you could potentially sneak through nodes in an embedquery there.

invisig0th
invisig0th previously approved these changes Jul 6, 2023
Copy link
Contributor

@Cisphyx Cisphyx left a comment

Choose a reason for hiding this comment

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

An embedquery will still allow nodes from other views through:
view.exec 4aaedfd7ce26c9617f623e0b73c85f79 { $x=${it:dev:str=foo } } | yield $x

@vEpiphyte vEpiphyte added bug and removed enhancement labels Jul 6, 2023
@vEpiphyte vEpiphyte merged commit 49c7ba0 into master Jul 6, 2023
@vEpiphyte vEpiphyte added this to the v2.141.0 milestone Jul 7, 2023
@vEpiphyte vEpiphyte deleted the feat_yield_safety branch July 7, 2023 14:28
@vEpiphyte vEpiphyte removed the reqChangelog requires changelog label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants