Fix show_deleted query param for /materializations, rename it show_inactive#1386
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
| ) | ||
| else: # Get just the active materializations on the current node revision | ||
| for materialization in node.current.materializations: # type: ignore | ||
| if not materialization.deactivated_at: # pragma: no cover |
There was a problem hiding this comment.
I think this code now would not give you the option of seeing just the current materialization regardless of their status. I wonder if it would be cleaner to have two params on this whole method:
show_deletedas before (although I thinkshow_inactivesounds better)include_all_revisionsas a param to scan previos node states.
I would lean to longer params/names that are obvious, rather than shorter ones that look nice.
There was a problem hiding this comment.
I see what you're saying. So show_inactive would basically ignore the deactivated_at value and include_all_revisions would iterate over all of the node revisions to pull the materialization. Let me update this.
There was a problem hiding this comment.
Yes, i think this now supports all the query options. Thanks!
agorajek
left a comment
agorajek
left a comment
There was a problem hiding this comment.
Just some unit tests to fix...
| if ( | ||
| not materialization.deactivated_at or show_inactive | ||
| ): # pragma: no cover | ||
| materializations.append( |
There was a problem hiding this comment.
Can this part be pulled into a separate function that operates on node_revision, whether it's current or an older revision?
| for materialization in node_revision.materializations: | ||
| if ( | ||
| not materialization.deactivated_at or show_inactive | ||
| ): # pragma: no cover |
There was a problem hiding this comment.
I think there is coverage for this line right?
There was a problem hiding this comment.
You're right, I'm not sure why I put this here. 🤔
samredai
changed the title
show_deleted query param for /materializations, rename it include_allshow_deleted query param for /materializations, rename it show_inactive
3 participants
Summary
When calling
/nodes/{node_name}/materializations?show_deleted=true, it wasn't returning all of the materializations for a given node because it only looks at the current version (vianode.current). This fixes that by pulling all materializations for all node revisions, only when that query param is enabled, otherwise the behavior remains the same.I also renamed it to
include_allsince previous versions of the node could have materializations that aren't deleted, but still can only be accessed via this flag, so viewing deleted materializations is just a subset of why this would be used. I don't think this query param is used anywhere so changing this shouldn't break anything, but let me know if I missed somewhere it is being used.Test Plan
Added
test_include_all_materialization_configs.make checkpassesmake testshows 100% unit test coverageDeployment Plan