-
Couldn't load subscription status.
- Fork 267
fix: Fix checking value in Golang ValidateJSONB #2383
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
base: main
Are you sure you want to change the base?
Conversation
|
@icbd is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
| s.l.Warn().Msg("retry count is nil, using task's current retry count") | ||
| } | ||
|
|
||
| if request.EventType == contracts.StepActionEventType_STEP_EVENT_TYPE_COMPLETED { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to double check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably only want this for completed / failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a full development environment, so I've only run unit tests on this change.
Could you help run an E2E test to confirm the change? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, will do tomorrow AM - CI should be a pretty good indicator though 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the core of this PR is changing \\u0000 to \u0000
Using bytes just improves performance.
I'm not sure if a couple new commits are directly related to this PR.
If it's not relevant, we can submit it first and rebase it.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the issue here isn't that the Validate method isn't working (it is) but we just weren't calling it every place we needed to. Is that not your understanding? It's easy to verify - if you change the string thing to bytes, those python tests will fail and you'll see errors on the engine. I'm not sure why it doesn't work, but it doesn't 🤷
Also, I'm not too worried about the bytes -> string conversion for performance here, although it's a fair point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit is what I want, everything else is optional.
JSON encoder will filter and resolve NUL(JSON encoder transfers \u0000 to \\u0000
\u0000 is one character,means NUL,pg will also reject it.
\\u0000 are six characters,are normal string.
so,you may check the invalid character NUL,instead of a normal str
1377b16 to
381272c
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
pkg/repository/jsonb.go
Outdated
| } | ||
|
|
||
| if strings.Contains(string(jsonb), "\u0000") { | ||
| if bytes.Contains(jsonb, []byte("\u0000")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi @icbd this didn't work so I reverted it - not completely sure why though tbh, it seemed fine at first glance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this broke a unit test btw, but I validated the behavior with an E2E test. Let me know if you disagree!
Description
hatchet/sdks/python/hatchet_sdk/worker/runner/runner.py
Lines 501 to 506 in 2a08cbf
This comment has explained the significance of this method, but the implementation of this method has problems.
This PR fixes the value of this check.
Fixes # (issue)
When the object returned normally contains "\u0000", the task will be reset to failure.
Source Code:
hatchet/internal/services/dispatcher/server_v1.go
Lines 557 to 562 in 5bf9f97
When the object returned error contains "\u0000", the error content is displayed normally (No check for ValidateJSONB).
Type of change
What's Changed