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

Load custom encoder only when runner enabled #6748

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Feb 6, 2024

What type of PR is this?

custom json encoders import, which was introduced in #6687 and fixed in #6741 includes all encoders which leads to E2E test failure as mongodb dependencies are not installed during E2E test execution, but ObjectId is used inside mongodb custom encoder. this PR filters out encoders of disabled query runners

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

"""Adapter for `json.dumps`."""

def __init__(self, **kwargs):
self.encoders = [

Choose a reason for hiding this comment

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

Just curious why is this an array? Is it possible to have result from multiple query runners at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not a result, these are custom json encoder functions for each query runner

Copy link

@clearnote01 clearnote01 Feb 7, 2024

Choose a reason for hiding this comment

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

How is there control then on what custom_json_encoder is actually used? For example, pg.py and mognodb.py both have custom_json_encoder defined. Then self.encoders will be [encoder_from_pg, encoder_from_mongodb].

In that case, say we had a query result from mongo which needed to be encoded, wouldn't the following code try to use both encoders, one by one and say if both encoders had different logic for encoding Timestamp, whichever encoder is first in list get to do the encoding?

 def default(self, o):
        for encoder in self.encoders:
            result = encoder(self, o)
            if result:
                return result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't matter which encoder is first, cause custom encoders are to process custom data types, provided by runner's lib, like ObjectId for mongodb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a fix for #6747

Copy link

@clearnote01 clearnote01 Feb 7, 2024

Choose a reason for hiding this comment

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

i don't get how your change isn't recursive? if encoder(self, o) returned an encoded string, then none of the elif condition in default(...) catch it, which would result in super().default(o) called again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

super().default(o) calls default method of json.JSONEncoder not redash.utils.json.JSONEncoder

Choose a reason for hiding this comment

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

thanks for clarification : )

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (ab39283) 63.41% compared to head (5b67199) 63.42%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6748      +/-   ##
==========================================
+ Coverage   63.41%   63.42%   +0.01%     
==========================================
  Files         162      162              
  Lines       13169    13173       +4     
  Branches     1819     1819              
==========================================
+ Hits         8351     8355       +4     
  Misses       4522     4522              
  Partials      296      296              
Files Coverage Δ
redash/utils/__init__.py 72.14% <100.00%> (+0.20%) ⬆️
redash/query_runner/cass.py 46.73% <60.00%> (+0.58%) ⬆️
redash/query_runner/pg.py 42.56% <37.50%> (+0.29%) ⬆️
redash/query_runner/mongodb.py 41.06% <33.33%> (+0.28%) ⬆️

@AndrewChubatiuk
Copy link
Contributor Author

this MR will fail cause it's now running against master, need to merge it to fix CI

@guidopetri
Copy link
Contributor

Hi Andrii, thanks for the PR. Obviously we'll need to fix CI first before considering this.

That being said, this is the third json-encoder-related PR I've seen but I'm still unclear on what is going on with these changes and what the fix is, as well as how it's been tested. Would you mind spending some time explaining these changes more in depth?

Copy link
Contributor

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

(placeholder)

@AndrewChubatiuk
Copy link
Contributor Author

previous json related PR fixed custom JSON encoders dynamic loading, but it didn't take into account if a runner is enabled, that's why tests in master are failing, cause it doesn't install mongodb dependencies before running cypress tests. I've ran cypress tests locally to validate for and instance with and without mongo dependencies

Copy link
Contributor

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

Hey, please don't put unrelated changes into a PR like this, it makes it hard to track what happens in a PR. Future programmers - you or I or someone else - will be very confused to see CI changes in a commit talking about json encoders.

@AndrewChubatiuk
Copy link
Contributor Author

Added this here, cause it fixes both CI and redash

@justinclift
Copy link
Member

justinclift commented Feb 6, 2024

@AndrewChubatiuk Like @guidopetri says, iit'd be better to have a PR that just fixes CI (eg changing sha hash or whatever), and the custom encoder stuff can go in a separate PR. 😄

@AndrewChubatiuk AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch from 21049c9 to 93578f8 Compare February 6, 2024 13:29
@AndrewChubatiuk
Copy link
Contributor Author

@justinclift @guidopetri #6749

@AndrewChubatiuk AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch 2 times, most recently from 4f41e05 to 1b132b4 Compare February 6, 2024 13:48
@AndrewChubatiuk AndrewChubatiuk mentioned this pull request Feb 6, 2024
10 tasks
@justinclift
Copy link
Member

justinclift commented Feb 6, 2024

@AndrewChubatiuk @guidopetri k, I've merged that other PR to fix the CI branch problem.

@justinclift
Copy link
Member

@AndrewChubatiuk Shouldn't this PR be passing CI now that the other one is merged and this one has been rebased?

@AndrewChubatiuk AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch 3 times, most recently from 5fe222b to 72555b2 Compare February 6, 2024 20:17
@AndrewChubatiuk
Copy link
Contributor Author

@justinclift pushed unit test fixes

@justinclift
Copy link
Member

@AndrewChubatiuk Excellent, thanks. 😄

@AndrewChubatiuk
Copy link
Contributor Author

@justinclift @guidopetri can we merge this to fix CI in master?


def __init__(self, **kwargs):
self.encoders = [
r.custom_json_encoder for r in query_runners.values() if hasattr(r, "custom_json_encoder") and r.enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice this line seems to be the only actual change in this PR. @AndrewChubatiuk , please undo all the other changes - e.g. creating this json.py file and all the import changes, as well as the classmethod additions. Work like this doesn't make it easy to review code, nor does it make it easy to follow code when investigating issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, please explain this change in the PR description in two or three paragraphs, providing:

  • context of why this change is necessary
  • what led to this change (including links to all previous related PRs)
  • what the current behavior is, and why it's wrong
  • and what the intended behavior is.

Additionally, please describe how you've tested it. If you only have manual tests, then please add some kind of automated test with pytest or unittest to make sure that this does not break again.

Copy link
Contributor Author

@AndrewChubatiuk AndrewChubatiuk Feb 8, 2024

Choose a reason for hiding this comment

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

i cannot make this change without moving JSONEncoder to a separate file. It will lead to circular import problem as query_runners variable, which is used to import custom json encoders is inside redash module, which uses redash.utils. JSONEncoder is inside redash.utils module, which uses redash module.
Regarding tests, I've tested it both manually and also e2e and unit tests have passed successfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guidopetri also updated PR description

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try making the import of query_runners that you need within the JSONEncoder definition? I believe that will circumvent the circular import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guidopetri commited in a way you want. you can check CI logs

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the current import location leads to a circular import. Please read my comment above more carefully - can we make the import within the JSONEncoder definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

@AndrewChubatiuk AndrewChubatiuk Feb 9, 2024

Choose a reason for hiding this comment

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

@guidopetri tried to put imports, which cause circular dependencies issues, inside classes, but it didn't work. moved query_runners to another module instead to have less changes comparing to moving json methods to a separate module

@AndrewChubatiuk AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch 4 times, most recently from b151747 to 12e3064 Compare February 8, 2024 05:23
@AndrewChubatiuk AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch 3 times, most recently from e5b8bdd to 3cd909e Compare February 9, 2024 19:11
@AndrewChubatiuk AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch from 3cd909e to 7e110a6 Compare February 9, 2024 19:16
Comment on lines +77 to +79
from redash.query_runner import query_runners

self.encoders = [r.custom_json_encoder for r in query_runners.values() if hasattr(r, "custom_json_encoder")]
Copy link
Contributor

Choose a reason for hiding this comment

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

As you can see, this change is much smaller and more readable than previously: only the essential was done to fix the issue at hand. We can move things away from the __init__.py files in a later PR, but for now we need to fix the CI, so we shouldn't do any refactor changes if they're not necessary.

@guidopetri
Copy link
Contributor

Merging now that we're passing the tests. The codecov isn't as important in this circumstance but we should improve it later.

@guidopetri guidopetri merged commit 939bec2 into getredash:master Feb 10, 2024
13 of 14 checks passed
@AndrewChubatiuk AndrewChubatiuk deleted the load-custom-encoder-only-when-runner-enabled branch February 10, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants