Add collections #1824
Add collections #1824
Conversation
alejandromumo
force-pushed
the
add_collections
branch
from
01e34fe
to
9233f72
Compare
| ret = db.session.execute(stmt).scalars().all() | ||
| return [type(self)(r) for r in ret] | ||
|
|
||
| def get_subcollections(self, max_depth=3): |
There was a problem hiding this comment.
this makes sense to have in CollectionTree API
|
|
||
| def create(self, identity, community_id, tree_slug, slug, title, query, **kwargs): | ||
| """Create a new collection.""" | ||
| current_communities.service.require_permission( |
There was a problem hiding this comment.
if it has a REST API, permissions could be added directly to the service so they can e.g. be overwritten by Zenodo to System
| def read(self, identity, id_): | ||
| """Get a collection by ID or slug.""" | ||
| collection = self.collection_cls.resolve(id_) | ||
| if not collection: | ||
| raise ValueError(f"Collection {id_} not found.") | ||
| if collection.community: | ||
| current_communities.service.require_permission( | ||
| identity, "read", community_id=collection.community.id | ||
| ) | ||
|
|
||
| return CollectionItem(collection) | ||
|
|
||
| def read_slug(self, identity, community_id, tree_slug, slug): | ||
| """Get a collection by slug.""" | ||
| current_communities.service.require_permission( | ||
| identity, "read", community_id=community_id | ||
| ) | ||
|
|
||
| ctree = CollectionTree.get_by_slug(tree_slug, community_id) | ||
| if not ctree: | ||
| raise ValueError(f"Collection tree {tree_slug} not found.") | ||
|
|
||
| collection = self.collection_cls.resolve(slug, ctree.id, use_slug=True) | ||
| if not collection: | ||
| raise ValueError(f"Collection {slug} not found.") | ||
|
|
||
| return CollectionItem(collection) |
There was a problem hiding this comment.
collapse into just one read that is smart enough, same as API get
consider depth as well
* collections: fix create api
alejandromumo
force-pushed
the
add_collections
branch
from
b61fa0d
to
7be1ad0
Compare
| @classmethod | ||
| def get_by_slug(cls, slug, tree_id): | ||
| """Get a collection by slug.""" | ||
| return cls.query.filter(cls.slug == slug, cls.tree_id == tree_id).one_or_none() |
There was a problem hiding this comment.
Do we return None, or should it fail when it's not found?
There was a problem hiding this comment.
I see the point of failing fast, however, I prefer to return None for separation of concerns. Whether the collection does not exist is an exception, I think it should be up to the business layer to decide. From a data point of view, it tried to fetch the collection but nothing was found.
I agree that we need to throw an exception at some point to have the presentation layer(s) acting accordingly (and that's currently missing in this PR). I will add that part in the next iteration since I refactored part of the public API / service layers from Alex's comments.
There was a problem hiding this comment.
Maybe None works in this case, but you will see soon its limitations. With a None return value there are 2 main problems (and maybe more):
- every time you call the func, you need to check if the return value is None or not. And the subsequent code will have to handle this case. So, as in your PR, you end up having
ifeverywhere. Not very elegant. - even if less applicable in this func, in general you can't differentiate different type of unhappy path.
An exception is much more elegant because the caller will have many more options on how to handle the unhappy path.
I don't really understand what are concerns are better separate with returning None.
No problem with solving this later on, in subsequent PRs.
| from .models import CollectionTree as CollectionTreeModel | ||
|
|
||
|
|
||
| class Collection: |
There was a problem hiding this comment.
Are we sure that this api layer is really needed, when the data model is not a classic record with a json schema? Can't the models be directly used in the service layer (as done in invenio-jobs for example)?
This class looks like a wrapper of the model, and the various methods can be moved to the model class.
There was a problem hiding this comment.
On one hand, I agree this class feels like a wrapper to the model, and it could be richer (e.g. I and Alex discussed storing the relation between collections in memory and then they could be "navigated" in memory).
However, some ideas that made me implement a public API for collections:
- Abstract some concepts about how we store collections in DB (materialized path). In my opinion, for the developer, things like
pathshould be abstracted and encapsulated under a "nice looking" API. - Related to the one above, adding some public-facing utilities to improve the usability of collections, e.g.
collection.ancestors,collection.children, create a collection based on a parent, resolving a collection byidorslug, community_idpair. dump/to_dictmethods for serialization.
There was a problem hiding this comment.
My 2 cents
I understand your reasoning. At the same time, with this approach, we will end up having a wrapper classes that have most of the attributes and methods duplicated from the encapsulated model. And then we add a couple of extra methods.
I would not know what is the separation of concerns or abstraction, and if I have to add a new feature (a new query), I would not know if I have to put in the API or in the Model. What is the criteria that tells me where to put it?
For the path, maybe, I don't see it for the moment in the class.
For your second point, it can be done in the model, I don't see the difference.
A model is already an API class. What has been done for records is actually very different and in that case very much needed: the model is loaded into a dict object, with many other extra methods.
If we start creating wrappers everywhere, we will end up having more complex code, and less DRY. As a comparison, it looks a little bit like the props drilling problem in React.
As an alternative, if you really feel like that we need to separate things, then in this case I would go with inheritance instead of composition.
To recap: I think that our final objective is to have the simplest possible code. If the majority of the APIs exposed by a model are intended to be used, I would use the model directly or create inheritance.
Composition and wrapping make sense to me when you want to hide the majority of the features provided by the wrapped object.
There was a problem hiding this comment.
Those are excellent points, I will keep them in mind in the next PR.
Thanks for the detailed explanation 🚀
|
|
||
| def create(self, identity, community_id, tree_slug, slug, title, query, **kwargs): | ||
| """Create a new collection.""" | ||
| current_communities.service.require_permission( |
There was a problem hiding this comment.
Maybe we should introduce a collection specific permissions instead of using the communities' ones?
can_update default value is CommunityOwners, is this correct?
There was a problem hiding this comment.
You are right, conceptually, communities are not required to have a collection (think about a collection on the repository level). However, the current state of this service is to require a community (this is our use case right now).
I agree that the collections' permission policy at some point has to be specific for collections, but it still needs more thought and the service to be adapted to it.
For a first launch, the permission can_update from communities is good since we don't want any user of a community creating collections for a community
| return CollectionItem(collection) | ||
|
|
||
| def read(self, identity, id_): | ||
| """Get a collection by ID or slug.""" |
There was a problem hiding this comment.
| """Get a collection by ID or slug.""" | |
| """Get a collection by ID.""" |
There was a problem hiding this comment.
This is going to be refactored in a second iteration when we add collections to the browse page. The idea is that service.read() is flexible enough that it can be done by ID or slug
| collection = self.collection_cls.create( | ||
| slug=slug, title=title, query=query, ctree=ctree, **kwargs | ||
| ) | ||
| return CollectionItem(collection) |
There was a problem hiding this comment.
create and add methods are missing the commit operations (for example uow.register(ModelCommitOp(...)))
There was a problem hiding this comment.
Will be added now :)
|
|
||
| def test_create(running_app, db, community, community_owner): | ||
| """Test collection creation via API.""" | ||
| tree = CollectionTree.create( |
There was a problem hiding this comment.
I suggest changing the test to read from the DB when asserting the creation of the collection [tree]. Given that the db commit is missing, they should fail.
closes #1820