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

🐛 Destination MySQL: fix problem with data containing nested JSON #4664

Closed
wants to merge 2 commits into from

Conversation

julianopiovezan
Copy link
Contributor

What

Fix #4583.

How

When a source has a text field containing a JSON, the double quotes of this field in the AirbyteRecordMessage must be escaped with a backslash, following the spec.

Here is a sample of a temp file created by writeBatchToFile

ee9e9984-083a-439b-8a1d-f9178181fb01,"{""id"":1,""topic"":""install"",""timestamp"":""2017-08-22T17:22:15Z"",""model"":""install"",""details"":""{\""name\"":\""Conferência Faturamento - Custo - Taxas - Margem - Resumo ano inicial até -2\"",\""description\"":null}""}",2021-07-08 00:58:59.889

The proposed solution is to use the ESCAPED BY clause of the MySQL LOAD DATA statement so the sequence \" will be treated as part of the field value.

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in the connector's spec
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Documentation updated
    • README.md
    • docs/SUMMARY.md if it's a new connector
    • Created or updated reference docs in docs/integrations/<source or destination>/<name>.
    • Changelog in the appropriate page in docs/integrations/.... See changelog example
    • docs/integrations/README.md contains a reference to the new connector
    • Build status added to build page
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

Connector Generator checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

@github-actions github-actions bot added the area/connectors Connector related issues label Jul 9, 2021
@marcosmarxm marcosmarxm requested a review from subodh1810 July 9, 2021 17:29
@marcosmarxm marcosmarxm changed the title Destination MySQL: fix problem with data containing nested JSON 🐛 Destination MySQL: fix problem with data containing nested JSON Jul 9, 2021
Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @julianopiovezan! LGTM to me, @subodh1810 can you validate it too?

@marcosmarxm
Copy link
Member

@julianopiovezan I also create #4671 to insert a unit test to validate and ensure this problem doesnt happen again for all jdbc-destination connector

@marcosmarxm
Copy link
Member

marcosmarxm commented Jul 9, 2021

Also already create PR #4671 to test and bump version of this modification.

@julianopiovezan
Copy link
Contributor Author

@julianopiovezan I also create #4671 to insert a unit test to validate and ensure this problem doesnt happen again for all jdbc-destination connector

Nice, thanks @marcosmarxm.

Copy link
Contributor

@subodh1810 subodh1810 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine. It would be great if we can add a test case for this

@dislazy
Copy link

dislazy commented Jul 14, 2021

This looks fine. It would be great if we can add a test case for this

I also encountered the same problem, I hope I can help solve it as soon as possible, thank you

@etsybaev
Copy link
Contributor

etsybaev commented Jul 19, 2021

Hope that will be fixed as part of #4825

@marcosmarxm
Copy link
Member

@etsybaev can we merge this and add test in #4825?

@etsybaev
Copy link
Contributor

etsybaev commented Jul 19, 2021

Hi @marcosmarxm this PR has been created 10 days ago, but probably author doesn't have time to finish it. My PR contains both fix, test and it's based on the latest master. I also have a bumped versions. So I would rather cancel this PR and merge it all at one shot to have a git history consistent (in my PR). What do you think?

@marcosmarxm
Copy link
Member

@etsybaev could we merge this and you pull master/solve conflicts in your side?

@etsybaev
Copy link
Contributor

etsybaev commented Jul 22, 2021

Hi @marcosmarxm. My PR with the fix, test and bumped versions was merged to master 2 days ago.#4825
So I assume we may close this one without future actions.

@marcosmarxm
Copy link
Member

@julianopiovezan thanks a lot for the contribution and guide the solution for the problem. I'm closing your PR because #4825 add tests to the problem. Hope for future contributions!

@julianopiovezan
Copy link
Contributor Author

Sure, no problem @marcosmarxm. I'm glad to help.

@julianopiovezan julianopiovezan deleted the issue-4583 branch July 28, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres to MySQL fails if source has a column with json data
5 participants