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

default_domain_table_size does not work correctly for values higher than 100 #1485

Closed
JanKoppe opened this issue Mar 28, 2023 · 6 comments · Fixed by #1491
Closed

default_domain_table_size does not work correctly for values higher than 100 #1485

JanKoppe opened this issue Mar 28, 2023 · 6 comments · Fixed by #1491
Assignees
Labels
bug / broken-feature Existing feature malfunctioning or broken mod / accepted This request has been accepted
Milestone

Comments

@JanKoppe
Copy link
Contributor

PDA version

0.4.0

Python version

3.10

Steps to Reproduce

  1. Create more than 100 zones.
  2. Go to Settings -> Basic
  3. Enter a higher value than 100 (e.g. 200) in default_domain_table_size and save.
  4. Go to Dashboard

Expected Behavior

A single dashboard page should show the configured amount of zones per page, as set in default_domain_table_size.

Observed Behavior

There will only ever be 100 zones shown per page. The pagination at the bottom seems to respect the new value in calculating the amount of available pages. Going to page two then only shows the zones 101-200, not the actual offset that should be used.

It seems that the value of default_domain_table_size is not being respected at all in the frontend.

@JanKoppe JanKoppe added the bug / broken-feature Existing feature malfunctioning or broken label Mar 28, 2023
@JanKoppe
Copy link
Contributor Author

JanKoppe commented Mar 28, 2023

Short investigation shows that PDA seems to be doing everything right - the pageLength option for the DataTables plugin is correctly being filled from the setting. I can verify that the correct setting is showing up in the rendered template. Still it does not work. This might be an issue with DataTables itself?

https://github.com/PowerDNS-Admin/PowerDNS-Admin/blob/v0.4.0/powerdnsadmin/templates/dashboard.html#L133

@JanKoppe
Copy link
Contributor Author

I think I found the issue.

https://github.com/PowerDNS-Admin/PowerDNS-Admin/blob/v0.4.0/powerdnsadmin/routes/dashboard.py#L144

PDA caps the search result at 100 rows. DataTables is correctly requesting 1000 rows, but only getting 100 back. When switching to "all", the length parameter to the dashboard.domains_custom route is set to -1, which will be respected.

Solution IMO would be to tie that hard cap to the default_domain_table_size setting, instead of hardcoding it to 100.

@JanKoppe JanKoppe changed the title default_domain_table_size only affects the pagination calculation, but not the actual amount of displayed rows default_domain_table_size does not work correctly for values higher than 100 Mar 28, 2023
@AzorianMatt
Copy link
Member

@JanKoppe thank you for your work on this! I am preparing to do a rebuild on the zone editor features to clean up a lot of poorly written functionality as it relates to this very area.

Stay tuned for big updates on that in a semi near future release!

@AzorianMatt AzorianMatt added the mod / reviewing This request is being reviewed label Mar 28, 2023
@JanKoppe
Copy link
Contributor Author

Would you be willing to accept a tiny patch for this with my suggested change, to make it more usable until the rework? :)

@AzorianMatt
Copy link
Member

@JanKoppe I welcome it!

All I ask is that when you make the change, be mindful of the fact that many current users are dealing with very large data sets, so change accordingly!

@AzorianMatt AzorianMatt added mod / accepted This request has been accepted and removed mod / reviewing This request is being reviewed labels Mar 29, 2023
@AzorianMatt AzorianMatt added this to the V0.4.1 milestone Mar 29, 2023
JanKoppe added a commit to BurdaForward/PowerDNS-Admin that referenced this issue Mar 29, 2023
The dashboard.domains_custom route was hardcoded to either return all
the domains, or at most 100, regardless of default_domain_table_size
setting.

Make this limit be dependent on default_domain_table_size instead.

The API will now limit to 100 or default_domain_table_size, whichever
one is higher. This is done to not break any seconday use-cases that
might depend on the hardcoded setting.
@JanKoppe
Copy link
Contributor Author

PR opened. For large users this should have no negative effect. The default was 10, you could set it up to 100, now you can set it to whatever you wish. If you have a giant dataset, you can just leave it at 100, and that limit will still be respected. There should be no performance degradation from this, just more options. I've tested this locally with 1500 domains, and I did not observe any performance degradation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / broken-feature Existing feature malfunctioning or broken mod / accepted This request has been accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants