Skip to content

Fix LookerHook serialize missing 1 argument error #34678

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

Merged
merged 5 commits into from
Oct 2, 2023

Conversation

cjonesy
Copy link
Contributor

@cjonesy cjonesy commented Sep 29, 2023

There was a ticket open for this back in March (#30169), it was closed out due to inactivity and there was a comment that it was related to an upstream issue in looker_sdk. I took a look at the issue in looker_sdk and it didn't quite seem like the same thing.

After digging in, I believe this is simply a typo in the hook's get_looker_sdk() method and the 40 was left off by mistake. Adding the 40 back on here fixes the issues in my local environment. I can't seem to find any reason it would be desirable to use serialize.serialize there, and in fact looker_sdk uses serialize40 here.

You can see here in the looker_sdk repo that during init we use serialize.serialize40, whereas in LookerHook we use serialize.serialize.

serialize.serialize points here, where it expects a converter to be passed in.

serialize.serialize40 points here, where there is a default converter set.

This becomes problematic when you want to do something with the sdk from LookerHook when calling a method using post, like create_query.

An example - when we try to do something like this, we get an error due to the missing converter:

def example():
  sdk = LookerHook("looker_conn").get_looker_sdk()

  query = sdk.create_query(
    body=models.WriteQuery(
      model="my_model",
      view="my_explore",
      fields=["field1"],
    )
  )

with DAG(...) as dag:
  PythonOperator(task_id="example", python_callable=example)

This generates the error: TypeError: serialize() missing 1 required keyword-only argument: 'converter'


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Sep 29, 2023
@utkarsharma2
Copy link
Contributor

@cjonesy Would it make sense to add a testcase to make sure you are no longer facing the issue Error: TypeError: serialize() missing 1 required keyword-only argument: 'converter' just to be sure and also ensuring such typo doesn't happen in future?

@cjonesy
Copy link
Contributor Author

cjonesy commented Sep 29, 2023

@cjonesy Would it make sense to add a testcase to make sure you are no longer facing the issue Error: TypeError: serialize() missing 1 required keyword-only argument: 'converter' just to be sure and also ensuring such typo doesn't happen in future?

@utkarsharma2 - Good catch! Tests have been added.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Looks good

@hussein-awala hussein-awala merged commit 562b98a into apache:main Oct 2, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Oct 2, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@hussein-awala
Copy link
Member

Congrats on your first commit 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants