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

feat: Provide a generic Tree.dump() function #1366

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions src/viur/core/prototypes/tree.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import json
import typing as t
from deprecated.sphinx import deprecated
from viur.core import utils, errors, db, current
Expand All @@ -7,6 +8,7 @@
from viur.core.cache import flushCache
from viur.core.skeleton import Skeleton, SkeletonInstance
from viur.core.tasks import CallDeferred
from viur.core.render.json.default import CustomJsonEncoder
from .skelmodule import SkelModule


Expand Down Expand Up @@ -1064,6 +1066,105 @@ def onCloned(self, skelType: SkelType, skel: SkeletonInstance, src_skel: Skeleto
if self.leafSkelCls:
self._clone_recursive("leaf", src_skel["key"], skel["key"], skel["parentrepo"])

# Helpers

def get_content(
self,
parententry: t.Optional[db.Key | int | str] = None,
depth: int = 99,
limit: int = 99,
_level: int = 1,
**kwargs
) -> t.Dict:
"""
Reads the content of a given parententry recursively into a dict.

:param parententry: Parententry to dump; If not given, the fuction tries to figure out a single parent entry.
:param depth: Depth to dump children.
:param limit: Limit of entries on each level.
"""
if not utils.string.is_prefix(self.render.kind, "json"):
raise errors.BadRequest("Can only use this function on JSON-based renders")
Comment on lines +1086 to +1087
Copy link
Member

Choose a reason for hiding this comment

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

Methods in a module should be render independent IMO. That's exactly the purpose of the renderer, to transform data from a generic to a specific output.

I would just fetch the render instances here and in dump() pass the result through the renderer.

This method could also be used for internal purposes to fetch data for non-json-renderers.


if not parententry:
repos = self.getAvailableRootNodes(**kwargs)

match len(repos):
case 0:
raise errors.Unauthorized()
case 1:
parententry = repos[0]["key"]
case _:
raise errors.NotAcceptable(f"Missing required parameter {'parententry'!r}")

# fetch the nodes
q = self.viewSkel("node").all()
q.mergeExternalFilter(kwargs | {"parententry": parententry})

if not (q := self.listFilter(q)):
raise errors.Unauthorized()

self._apply_default_order(q)

ret = []
for skel in q.fetch(limit=limit):
node = self.render.renderSkelValues(skel)
node["_skeltype"] = "node"

# recurse into children
if _level < depth:
node["_children"] = self.get_content(skel["key"], depth=depth, limit=limit, _level=_level + 1, **kwargs)

ret.append(node)

# fetch the leafs (when this is a tree)
if self.leafSkelCls:
q = self.viewSkel("leaf").all()
q.mergeExternalFilter(kwargs | {"parententry": parententry})

if not (q := self.listFilter(q)):
raise errors.Unauthorized()

self._apply_default_order(q)

ret += [
self.render.renderSkelValues(skel) | {"_skeltype": "leaf"}
for skel in q.fetch(limit=limit)
]

return ret

@exposed
def dump(
Copy link
Member

Choose a reason for hiding this comment

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

This should not be under Helpers, but below the other @exposed methods.

self,
parententry: t.Optional[db.Key | int | str] = None,
depth: int = 3,
limit: int = 30,
**kwargs
) -> str:
"""
Dumps a Tree's content as JSON, based on a parententry.

This function is very limited in its bounds, and shall only be used for quick retrieval of small tree structures
and for debugging purposes.

:param parententry: Parententry to dump; If not given, the fuction tries to figure out a single parent entry.
:param depth: Depth to dump children. Can only be a maxiumum of 3.
:param limit: Limit of entries on each level, can be maximum of 30 per level and type.
"""
if not utils.string.is_prefix(self.render.kind, "json"):
raise errors.BadRequest("Can only use this function on JSON-based renders")
Comment on lines +1155 to +1156
Copy link
Member

Choose a reason for hiding this comment

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

This is against the modul/renderer concept.

A method in the module should work with any renderer. The logic and the preprocessing take place generically in the module, the final processing into the target format in the renderer.
You have to make this method generic.

If it really only works / should only work within a renderer, you must implement it directly in this render (cf. ViRenderer's getStructure)


current.request.get().response.headers["Content-Type"] = "application/json"

if depth < 1 or depth > 3:
raise errors.NotAcceptable("'depth' out of bounds")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have already mentioned it, why not print the borders here?


if limit < 1 or limit > 30:
Comment on lines +1160 to +1163
Copy link
Member

@sveneberth sveneberth Jan 29, 2025

Choose a reason for hiding this comment

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

in my opinion, this is an arbitrary limit without any logical derivation or basis for calculation.

I assume you want to prevent the request from becoming too large with large trees. But that is simply too arbitrary for any context.

Furthermore, you underestimate polynomial growth. Your limit of $30^3 = 27000$ is, with the right data, beyond what should be processed within a request.
Whereas the extension by one more level, but a reduction of the limit to 5 would still be a perfectly conceivable amount: $5^4 = 625$.
While the theoretical worst-case scenario is unlikely to be used in practice, it is quite realistic to have more than 3 levels and more than 30 entries on at least one (probably the last level; the leafs).

I would therefore suggest offering the limitation in case you only want to display the first n entries per level in your view and want to reload the rest, but not limiting it so strictly.
In addition, I would add an absolute limit so that a maximum of, for example, 1000 entries, regardless of their level and number, can be loaded with one request. However, this limit is then not available for the frontend, but only a (configurable) limitation by the backend.

raise errors.NotAcceptable("'limit' out of bounds")
Copy link
Contributor

Choose a reason for hiding this comment

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

And here ?


return json.dumps(self.get_content(parententry, limit=limit, depth=depth, **kwargs), cls=CustomJsonEncoder)


Tree.vi = True
Tree.admin = True
Loading