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

Source Quickbooks: Number data types are being transformed to strings #4292

Closed
maciej-nedza opened this issue Jun 23, 2021 · 3 comments · Fixed by #4986
Closed

Source Quickbooks: Number data types are being transformed to strings #4292

maciej-nedza opened this issue Jun 23, 2021 · 3 comments · Fixed by #4986

Comments

@maciej-nedza
Copy link
Contributor

Expected Behavior

According to docs supported streams should leave number fields types as numbers in output schema.

Current Behavior

Most of number fields are being transformed to strings.
It's probably caused by these fields in quickbooks-tap being defined as

      "format": "singer.decimal",
      "type": [
        "null",
        "string"
]

Steps to Reproduce

  1. Create quickbooks source. Quickbooks lets you create free trial account with demo organisation already holding some test data.
  2. Create connection between quickbooks source and any destination.
  3. Check the source schema, e. g. Invoices stream and TotalAmt field. It's type is set to string, when it should be a number.

Severity of the bug for you

Medium

Airbyte Version

0.26.2-alpha

Connector Version (if applicable)

0.1.1

@maciej-nedza maciej-nedza added the type/bug Something isn't working label Jun 23, 2021
@maciej-nedza maciej-nedza changed the title Quickbooks number data types are being sent as strings Source Quickbooks: Number data types are being transformed to strings Jun 23, 2021
@sherifnada sherifnada added area/connectors Connector related issues lang/python labels Jun 25, 2021
@sherifnada sherifnada added this to the Connectors July 23, 2021 milestone Jul 9, 2021
@Zirochkaa
Copy link
Contributor

@sherifnada login/password are not in Latspass, only test creds.

@Zirochkaa Zirochkaa self-assigned this Jul 12, 2021
@Zirochkaa
Copy link
Contributor

Zirochkaa commented Jul 12, 2021

Are the account test credentials available in Lastpass? - yes

Task breakdown

  • Go through each stream's schema in sample_files/configured_catalog.json and update fields types according to docs (~7-8h because streams have large schemas, file contains 4.5k+ rows)
  • Add acceptance tests (SAT), at least spec, connection, discovery and basic_read tests (basic_read will check types of fields) (~4h)

Note - here is a link to singer code which do request and just yield records, meaning that singer doesn't apply any transformation. So the reason must be in schemas. All schemas were taken from here and for some properties there is the following block:

"format": "singer.decimal",
  "type": [
    "null",
    "string"
  ]

Airbyte doesn't recognize singer.decimal format so we need to remove format field and change type to ["null", "number"] for all properties which actually have number type.
And because of above explanation we need to add SAT first because it will tell us which streams have such error.

Link to task in zira.

@Zirochkaa Zirochkaa removed their assignment Jul 12, 2021
@sherifnada sherifnada added the priority/high High priority label Jul 21, 2021
@vitaliizazmic vitaliizazmic self-assigned this Jul 22, 2021
@vitaliizazmic
Copy link
Contributor

@Zirochkaa Quickbooks it's Intuit product. We can use its login and password, which is presented in Lastpass.

@sherifnada sherifnada removed this from the Connectors August 6th milestone Aug 6, 2021
@sherifnada sherifnada added this to the Connectors August 20th milestone Aug 6, 2021
vitaliizazmic added a commit that referenced this issue Aug 10, 2021
* Source Quickbooks #4292 - migrate to Airbyte CDK

* Source Quickbooks #4292 - use singer tap fork

* Source Quickbooks #4292 - remove requirements.txt

* Source Quickbooks #4292 - enable SAT except incremental test

* Source Quickbooks #4292 - remove unnecessary files

* Source Quickbooks #4292 - update cursor_field

* Source Quickbooks #4292 - commented out incremental test

* Source Quickbooks #4292 - enable incremental test

* Source Quickbooks #4292 - fix build.gradle and acceptance-test-docker.sh

* Source Quickbooks #4292 - update fork repo

* Source Quickbooks #4292 - bump version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants