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

Serialize the panels #1826

Open
wants to merge 9 commits into
base: serializable
Choose a base branch
from

Conversation

tim-schilling
Copy link
Contributor

This is a draft PR. I'm keeping my on-going work in this branch. I found that my efforts to close #1807 and #1808 quickly spiraled into a major refactor of core logic.

This currently handles the SQLPanel which required the following changes:

  • Drop raw_params entirely. We'll need to serialize the parameters every time now so we can't rely on the original params
  • Refactor the SQL queries to use a djdt_query_id which uniquely identifies each query. The SQL forms now reference this id and avoid passing SQL to be executed.
  • Move the formatting logic of SQL queries back to just before rendering. Otherwise we're rendering and storing the html in the store (I could be convinced I'm over-engineering for the future)

In addition this also changed our design for Panel

  • load_stats_from_store - Generates a Panel instance with the passed in data.
  • Panel.panel_id is now a class member to avoid needing to instantiate a panel to programatically get the panel id. With the new focus on fetching data for a specific panel given its ID, this became more of a need

tim-schilling and others added 5 commits August 20, 2023 21:23
This matches the new naming defined in the store module and will make
things eaiser to change moving forward.

This will break anything using the store internally causing issues for
third party packages.
The remainder of the work is to fix the individual panels' serialization errors.
This avoids needing to create an instance of a panel to get its
panel ID.
@tim-schilling
Copy link
Contributor Author

I think this is at a point where we can add some async tests to confirm that this approach will work or not.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Thanks!

  • I like the change from store IDs to request IDs.
  • It's nice that we no longer accept SQL strings from the client and execute them (even with signing)

I'm not totally sure, is this (somewhat) backwards compatible for third party panels? This wouldn't be an argument against it, just something to clarify.

tests/panels/test_sql.py Show resolved Hide resolved
@tim-schilling
Copy link
Contributor Author

is this (somewhat) backwards compatible for third party panels?

While it doesn't break any documented APIs for the panel to function itself, this version is not backwards compatible since it loads the content for the panel from the store (render_panel view). That will need to be made backwards compatible to make this generally backwards compatible.

@tim-schilling
Copy link
Contributor Author

@matthiask I'm back to forcing everything that can't be serialized to JSON to a str. Otherwise we're trying to do it manually per panel which will involve a function that's likely going to do similar object and collection iteration as json.dumps().

The alternative here is to inspect and iterate over every collection and
object passed around. This avoids having to reinvent the wheel in that
scenario.
Any instance attributes shouldn't be used because they can't be
relied upon for historical purposes. Especially when it comes to
the titles and nav titles.
@tim-schilling tim-schilling marked this pull request as ready for review September 5, 2023 01:27
@tim-schilling
Copy link
Contributor Author

This assumes we'll have some shared memory store for the cache hence the change to should_render_panels

@salty-ivy
Copy link
Contributor

salty-ivy commented Mar 5, 2024

from the above changes in consideration in consideration I was able able to run example with async calls
image

image

however I believe this async example would also work without serialised panels.

@tim-schilling
Copy link
Contributor Author

however I believe this async example would also work without serialised panels.

Why is that?

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.

None yet

3 participants