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

Support joins within OrderBy #404

Open
Skelmis opened this issue Aug 11, 2024 · 3 comments
Open

Support joins within OrderBy #404

Skelmis opened this issue Aug 11, 2024 · 3 comments

Comments

@Skelmis
Copy link
Contributor

Skelmis commented Aug 11, 2024

Currently it would appear that sorting based off of join's fails within the admin interface.

Example table code:

class ProjectAutomation(Table):
    id = UUID(primary_key=True, default=uuid.uuid4, index=True)
    project = ForeignKey(
        LazyTableReference("Project", app_name="home"),
        index=True,
        unique=True,
        help_text="The project this automation is for",
    )

class Project(Table):
    id = UUID(primary_key=True, default=uuid.uuid4, index=True)
    owner = ForeignKey(BaseUser, index=True, help_text="Who owns this project")

Code within create_admin that errors:

        order_by=[
            OrderBy(ProjectAutomation.project.owner),
        ],

Traceback cut to only Piccolo bits:

  File "/home/skelmis/.cache/pypoetry/virtualenvs/hermes-nZAhPr0C-py3.10/lib/python3.10/site-packages/piccolo_api/fastapi/endpoints.py", line 137, in get
    return await piccolo_crud.root(request=request)
  File "/home/skelmis/.cache/pypoetry/virtualenvs/hermes-nZAhPr0C-py3.10/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 606, in root
    return await self.get_all(request, params=params)
  File "/home/skelmis/.cache/pypoetry/virtualenvs/hermes-nZAhPr0C-py3.10/lib/python3.10/site-packages/piccolo_api/crud/validators.py", line 129, in inner_coroutine_function
    return await function(*args, **kwargs)
  File "/home/skelmis/.cache/pypoetry/virtualenvs/hermes-nZAhPr0C-py3.10/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 832, in get_all
    split_params = self._split_params(params)
  File "/home/skelmis/.cache/pypoetry/virtualenvs/hermes-nZAhPr0C-py3.10/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 694, in _split_params
    column = self._get_column(column_name=sub_value)
  File "/home/skelmis/.cache/pypoetry/virtualenvs/hermes-nZAhPr0C-py3.10/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 1101, in _get_column
    raise ValueError("Max join depth exceeded")
ValueError: Max join depth exceeded
@dantownsend
Copy link
Member

dantownsend commented Aug 11, 2024

Yeah, I was caught out by this one recently too.

Piccolo Admin is built on PiccoloCRUD, which has a max_joins argument:

https://github.com/piccolo-orm/piccolo_api/blob/cd4a8f4fbc66845128ececb3c152e009cfe66689/piccolo_api/crud/endpoints.py#L183

It's set to 0 by default. In Piccolo Admin we should expose this as a configurable value, or just set it to a value greater than 0 by default, because users of Piccolo Admin can be trusted.

The reason the max_joins 'feature' exists is just in case someone exposes PiccoloCRUD on a public endpoint, and a user crafts a really expensive query involving lots of joins (or they're able to join on tables we don't want exposing). But it's not an issue with Piccolo Admin, as the users are admins, and it just ends up being a pain.

@dantownsend
Copy link
Member

I had a think about it, and rather than exposing max_joins as another option in TableConfig, we can just set it automatically for now, based on the order_by value. I think it's the only thing in Piccolo Admin which requires joins.

@Skelmis
Copy link
Contributor Author

Skelmis commented Aug 11, 2024

There also appears to some weirdness surrounding joins within visible columns but I haven't played with them yet. This setting may also simply solve it also

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

No branches or pull requests

2 participants