Fix GetWorkflowExecution in PostgreSQL #3816
Conversation
| @@ -303,11 +303,11 @@ func (s *visibilitySuite) TestDeleteSelect() { | |||
| s.NoError(err) | |||
| s.Equal(0, int(rowsAffected)) | |||
|
|
|||
| selectFilter := sqlplugin.VisibilitySelectFilter{ | |||
| getFilter := sqlplugin.VisibilityGetFilter{ | |||
There was a problem hiding this comment.
Why are we using Get instead of Select here, now? Also, if we are doing that, we should rename the tests accordingly.
There was a problem hiding this comment.
Hm, I could just leave unchanged. SelectFilter with only namespace and run id will do a Get anyway under the hood.
There was a problem hiding this comment.
I think we should leave this unchanged, at least in this PR
There was a problem hiding this comment.
Actually, now I remember why I changed.
It's because in the new code, I made it not return an error in this case.
Currently, since Select calls Get in this case, it returns an error if the row is not found, which is kind of misleading because it's a Select.
Also, there's no actual use case of Select for the (namespace, run id) in the code, only in these tests.
I'll leave unchanged in this PR, and move to where I actually need this.
rodrigozhou
force-pushed
the
fix-postgres-get-wf-execution
branch
from
53db9ff to
ee060ae
Compare
What changed?
In PostgreSQL, fix trimming run id in
GetWorkflowExecution.Refactored some code, and added new unit tests to cover
GetWorkflowExeccutionfunction.Why?
In PostgreSQL, when doing
SELECT, the run id has to be trimmed as it returns with trailing spaces. The existing select function is doing it, but theGetWorkflowExecutionwas missing.How did you test it?
New unit tests.
Potential risks
None,
GetWorkflowExecutionis only used when removing SA and memo from mutable state is enabled (currently not used anywhere).Is hotfix candidate?
No.