Skip to content
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

Serialize Undefined values to JSON for rpc requests #3687

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Aug 3, 2021

resolves #3464

Description

The rpc server uses a custom JSON encoder that doesn't know how to encode Undefined variables. As a result, the rpc server returns a 500 error w/ a RecursionError if an rpc response contains an Undefined object. See #3464 (comment) for a full diagnosis of the issue.

This PR addresses the issue by teaching the dbt JSON Encoder how to serialize an Undefined object to JSON - specifically by serializing it out as an empty string.

Note: need to update the changelog on this one still...

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Aug 3, 2021
@drewbanin drewbanin temporarily deployed to Redshift August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Redshift August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Snowflake August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Snowflake August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Postgres August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Bigquery August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Bigquery August 3, 2021 17:29 Inactive
@drewbanin drewbanin temporarily deployed to Postgres August 3, 2021 17:54 Inactive
@drewbanin drewbanin temporarily deployed to Bigquery August 3, 2021 17:54 Inactive
@drewbanin drewbanin temporarily deployed to Bigquery August 3, 2021 17:54 Inactive
@drewbanin drewbanin temporarily deployed to Redshift August 3, 2021 17:54 Inactive
@drewbanin drewbanin temporarily deployed to Redshift August 3, 2021 17:54 Inactive
@drewbanin drewbanin temporarily deployed to Snowflake August 3, 2021 17:54 Inactive
@drewbanin drewbanin temporarily deployed to Snowflake August 3, 2021 17:54 Inactive
@drewbanin
Copy link
Contributor Author

is 0.21.0 the right place for this in the changelog? And more generally... how would I know which release a given commit merged to develop is going out in?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 4, 2021

is 0.21.0 the right place for this in the changelog? And more generally... how would I know which release a given commit merged to develop is going out in?

Yes, 0.21.0 is good. Since we've cut the first v0.21 prerelease (0.21.0-b1), thereby creating the 0.21.latest branch, you'll want to squash-merge into develop and then cherry-pick the squashed commit onto 0.21.latest.

Generally, you put your changelog entry under the release where that PR's changes are going out. If your change were for v0.20.1 instead of v0.21, you'd add a changelog entry under v0.20.1, and then also backport to 0.20.latest.

@leahwicz Does that sound right to you?

@leahwicz
Copy link
Contributor

leahwicz commented Aug 4, 2021

Yes that is correct

@drewbanin
Copy link
Contributor Author

Yes, 0.21.0 is good. Since we've cut the first v0.21 prerelease (0.21.0-b1), thereby creating the 0.21.latest branch, you'll want to squash-merge into develop and then cherry-pick the squashed commit onto 0.21.latest.

Huh - is that what we ask external contributors to do too? That feels... onerous... to me!

@drewbanin
Copy link
Contributor Author

Sorry - I hope I'm not being dense here - I just want to make sure I'm doing the right thing. I should squash+merge this into develop, then open a new PR to cherry pick the commit into 0.21.latest separately?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 6, 2021

Huh - is that what we ask external contributors to do too? That feels... onerous... to me!

We ask all external contributors to open PRs targeting develop. We determine which upcoming release the change will be going into, and we ask the external contributor to add a changelog entry under that release. When the PR is ready, we take care of squash-merging into develop and opening a back-port PR to the appropriate x.y.latest branch.

I should squash+merge this into develop, then open a new PR to cherry pick the commit into 0.21.latest separately?

That's right! I'm also happy to merge this PR and take it from there, let me know :)

@drewbanin
Copy link
Contributor Author

@jtcohen6 if you don't mind helping me out with that, it would be supremely helpful!

@jtcohen6 jtcohen6 merged commit 4364295 into develop Aug 10, 2021
@jtcohen6 jtcohen6 deleted the fix/rpc-serialize-undefined branch August 10, 2021 01:26
jtcohen6 pushed a commit that referenced this pull request Aug 10, 2021
* (#3464) Serialize Undefined values to JSON for rpc requests

* Update changelog, fix typo
jtcohen6 added a commit that referenced this pull request Aug 10, 2021
* (#3464) Serialize Undefined values to JSON for rpc requests

* Update changelog, fix typo

Co-authored-by: Drew Banin <drew@fishtownanalytics.com>
kwigley pushed a commit that referenced this pull request Sep 17, 2021
* (#3464) Serialize Undefined values to JSON for rpc requests

* Update changelog, fix typo

Co-authored-by: Drew Banin <drew@fishtownanalytics.com>
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.

Defined dict with malformed result from fromyaml() passed into log() crashes dbt
3 participants