Fix widget id stability #7003
Fix widget id stability #7003
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
`parsed_value` is always set in the function if `v` has the required type, but codeql doesn't understand that. Use direct returns so it sees the impossible case as an implicit None return, which is valid.
It has a different meaning than for most widgets.
There was a problem hiding this comment.
Do you think it makes sense to write tests for each widget that does the following:
patch compute_widget_id
call widget function
assert that compute_widget_id includes all args minus the disallowed for the widget (eg. disabled.
My fear is worrying that a new field is added, but not added to the computer widget id. This test would at least fail here and cause a conversation to be had on whether it should be disallowed or just ensured it's added to the compute_widget_id
We could write those tests, but it seems annoying, and I'm not sure it adds that much value. I expect it to be easy to remember to update the call, and any e2e test that has multiple copies of a widget with just that argument differing will get a duplicate id error (though maybe that won't happen because they'll have different labels? unsure). Even if we decide it would be worth having those tests, I don't want the PR to be blocked on them. I'd rather they be a quick followup. |
Cool. I'll wait to merge it until tomorrow, to give Lukas a chance to do a closer check of the data editor id calculation, since I want his verification that we're using the arrow table. |
I think we won't get any e2e test coverage for this since most of our tests have widgets with different labels. There's probably a clever way to write these tests without ridiculous amounts of code duplication using |
Now that we've merged #7003, some of the comments we have about when certain fields are written to widget protos are now stale. While removing these comments, we also move a few lines around so that lines where we're writing to widget protobufs are grouped.
…amlit#7093) Now that we've merged streamlit#7003, some of the comments we have about when certain fields are written to widget protos are now stale. While removing these comments, we also move a few lines around so that lines where we're writing to widget protobufs are grouped.
…amlit#7093) Now that we've merged streamlit#7003, some of the comments we have about when certain fields are written to widget protos are now stale. While removing these comments, we also move a few lines around so that lines where we're writing to widget protobufs are grouped.
…amlit#7093) Now that we've merged streamlit#7003, some of the comments we have about when certain fields are written to widget protos are now stale. While removing these comments, we also move a few lines around so that lines where we're writing to widget protobufs are grouped.
Describe your changes
Fix existing widget id instability in slider, document how to safely access session state in widgets, and have
register_widgetcheck some common fields that shouldn't be used for a widget's identity.GitHub Issue Link (if applicable)
Testing Plan
Some additional python unit tests should be added
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.