Load custom encoder only when runner enabled #6748

AndrewChubatiuk · 2024-02-06T11:52:41Z

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

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)

clearnote01 · 2024-02-06T12:06:13Z

redash/utils/json.py

+    """Adapter for `json.dumps`."""
+
+    def __init__(self, **kwargs):
+        self.encoders = [


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

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

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

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

I've pushed a fix for #6747

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?

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

thanks for clarification : )

codecov · 2024-02-06T12:08:56Z

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 · 2024-02-06T12:10:25Z

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

guidopetri · 2024-02-06T12:16:54Z

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?

guidopetri

(placeholder)

AndrewChubatiuk · 2024-02-06T12:21:21Z

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

guidopetri

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 · 2024-02-06T12:28:03Z

Added this here, cause it fixes both CI and redash

justinclift · 2024-02-06T13:17:17Z

@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 · 2024-02-06T13:31:22Z

@justinclift @guidopetri #6749

justinclift · 2024-02-06T17:16:15Z

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

justinclift · 2024-02-06T17:34:50Z

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

AndrewChubatiuk · 2024-02-06T20:18:12Z

@justinclift pushed unit test fixes

justinclift · 2024-02-07T04:06:53Z

@AndrewChubatiuk Excellent, thanks. 😄

AndrewChubatiuk · 2024-02-07T19:16:30Z

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

guidopetri · 2024-02-08T02:43:25Z

redash/utils/json.py

+
+    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


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.

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.

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

@guidopetri also updated PR description

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

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

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?

@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

guidopetri · 2024-02-10T02:21:42Z

redash/utils/__init__.py

+        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")]


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 · 2024-02-10T02:49:56Z

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

github-actions and others added 3 commits February 1, 2024 01:29

Snapshot: 24.02.0-dev

5a2daa4

Merge branch 'getredash:master' into master

6000d75

Merge branch 'getredash:master' into master

fc90c47

clearnote01 reviewed Feb 6, 2024

View reviewed changes

guidopetri requested changes Feb 6, 2024

View reviewed changes

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

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 mentioned this pull request Feb 6, 2024

Changed checkout commit #6749

Merged

10 tasks

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

guidopetri requested changes Feb 8, 2024

View reviewed changes

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

masayuki038 mentioned this pull request Feb 8, 2024

Fix columns duplication on MongoDB Query Runner #6640 #6641

Open

5 tasks

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

load encoders only for enabled runners

7e110a6

AndrewChubatiuk force-pushed the load-custom-encoder-only-when-runner-enabled branch from 3cd909e to 7e110a6 Compare February 9, 2024 19:16

try importing within init

5b67199

guidopetri reviewed Feb 10, 2024

View reviewed changes

guidopetri merged commit 939bec2 into getredash:master Feb 10, 2024
13 of 14 checks passed

AndrewChubatiuk deleted the load-custom-encoder-only-when-runner-enabled branch February 10, 2024 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load custom encoder only when runner enabled #6748

Load custom encoder only when runner enabled #6748

AndrewChubatiuk commented Feb 6, 2024 •

edited

clearnote01 Feb 6, 2024

AndrewChubatiuk Feb 6, 2024

clearnote01 Feb 7, 2024 •

edited

AndrewChubatiuk Feb 7, 2024

AndrewChubatiuk Feb 7, 2024

clearnote01 Feb 7, 2024 •

edited

AndrewChubatiuk Feb 7, 2024

clearnote01 Feb 7, 2024

codecov bot commented Feb 6, 2024 •

edited

AndrewChubatiuk commented Feb 6, 2024

guidopetri commented Feb 6, 2024

guidopetri left a comment

AndrewChubatiuk commented Feb 6, 2024

guidopetri left a comment

AndrewChubatiuk commented Feb 6, 2024

justinclift commented Feb 6, 2024 •

edited

AndrewChubatiuk commented Feb 6, 2024

justinclift commented Feb 6, 2024 •

edited

justinclift commented Feb 6, 2024

AndrewChubatiuk commented Feb 6, 2024

justinclift commented Feb 7, 2024

AndrewChubatiuk commented Feb 7, 2024

guidopetri Feb 8, 2024

guidopetri Feb 8, 2024

AndrewChubatiuk Feb 8, 2024 •

edited

AndrewChubatiuk Feb 8, 2024

guidopetri Feb 9, 2024

AndrewChubatiuk Feb 9, 2024

guidopetri Feb 9, 2024

AndrewChubatiuk Feb 9, 2024

AndrewChubatiuk Feb 9, 2024 •

edited

guidopetri Feb 10, 2024

guidopetri commented Feb 10, 2024

		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")]

Load custom encoder only when runner enabled #6748

Load custom encoder only when runner enabled #6748

Conversation

AndrewChubatiuk commented Feb 6, 2024 • edited

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clearnote01 Feb 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clearnote01 Feb 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2024 • edited

Codecov Report

AndrewChubatiuk commented Feb 6, 2024

guidopetri commented Feb 6, 2024

guidopetri left a comment

Choose a reason for hiding this comment

AndrewChubatiuk commented Feb 6, 2024

guidopetri left a comment

Choose a reason for hiding this comment

AndrewChubatiuk commented Feb 6, 2024

justinclift commented Feb 6, 2024 • edited

AndrewChubatiuk commented Feb 6, 2024

justinclift commented Feb 6, 2024 • edited

justinclift commented Feb 6, 2024

AndrewChubatiuk commented Feb 6, 2024

justinclift commented Feb 7, 2024

AndrewChubatiuk commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewChubatiuk Feb 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewChubatiuk Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guidopetri commented Feb 10, 2024

AndrewChubatiuk commented Feb 6, 2024 •

edited

clearnote01 Feb 7, 2024 •

edited

clearnote01 Feb 7, 2024 •

edited

codecov bot commented Feb 6, 2024 •

edited

justinclift commented Feb 6, 2024 •

edited

justinclift commented Feb 6, 2024 •

edited

AndrewChubatiuk Feb 8, 2024 •

edited

AndrewChubatiuk Feb 9, 2024 •

edited