Feature/collection api #181
Conversation
delete obsolete file scaffold collection model
backend/collect/utils.py
Outdated
| def _name_to_slug(name: str) -> str: | ||
| no_spaces = lambda s: re.sub(r'\s+', '_', s) | ||
| clean = lambda s: re.sub(r'[^a-z0-9\-_]', '', s) | ||
| strip_start = lambda s: re.sub(r'^\W+', '', s) | ||
| strip_end = lambda s: re.sub(r'\W+$', '', s) | ||
|
|
||
| return strip_end(strip_start(clean(no_spaces(str.lower(name))))) | ||
|
|
||
|
|
||
| def collection_uri(name: str): | ||
| id = _name_to_slug(name) | ||
| return URIRef(settings.RDF_NAMESPACE_ROOT + 'collections/' + id) |
There was a problem hiding this comment.
This constructs the URI for a collection based on the "human" name, i.e. "My collection" will be stored as my_collection. This simplifies the frontend UI, which is nice for an initial version, though it may lead to some annoyances long-term.
There was a problem hiding this comment.
URIs need to be unique, so the request will fail if the URI already exists. This isn't really a problem right now, but if a group of people is enthusiastically adding data on the same topics, conflicts may become more common.
I think there are some minor annoyances that can pop up if you get these warnings:
- You can get a response that the URI is taken while the existing collection is privately owned by another user. This makes technical sense, but it feels odd.
- Names don't need to be unique but URIs do, so this can feel un-intuitive. For instance, if "My collection" /
my_collectionexists, you can get the name you want by creating "My collection 2" /my_collection_2, and then renaming it to "My collection" (which will preserve the URImy_collection_2). - "my collection" and "My Collection :)" would both be encoded as
my_collection, so the user may feel like they don't have full control over the URI. Which is annoying if their collection gets rejected on the basis of the URI.
There was a problem hiding this comment.
Maybe number slugs automatically, then?
|
Current status: this PR is ready for review; I've made it a draft because it should only be merged in combination with a frontend update. Request for review: I recommend exploring the browsable API and/or looking at The technical implementation is still a bit experimental, but quite well-tested, so it should work well enough for a first version. Suggestions/comments are welcome but addressing them may be better left until later. |
|
@lukavdplas I merged Unfortunately, I cannot get past the migrations of the projects app. It gets stuck on number 2, log below. Does this look familiar to you and if so, do you know how to address it? The |
|
It does. This happens because migrations use a reconstruction of the model based on the migration history, rather than the actual class. So this migration tried to access the custom method This is caused by a mistake in the source code, I'll update it and let you know. |
|
Okay, I've updated it and it looks like the migrations run fine now. I'm not very happy with the solution (importing the actual model class during the migration) but it works. |
|
Getting one for |
|
Ah, that should be fixed now! Thanks for trying all this out btw 😅 |
There was a problem hiding this comment.
I was a bit confused by this comment:
Current status: this PR is ready for review; I've made it a draft because it should only be merged in combination with a frontend update.
The pull request compares to feature/rdf-modeling-utils, not develop. Surely, you are planning to build the feature/rdf-modeling-utils branch incrementally, and the current branch does not need to be complete with frontend adaptations before you merge it?
Side note: even on develop, I wouldn't necessarily be against you merging this without corresponding frontend changes. We just did something like that in #174. As far as I'm concerned, only on master/main does everything need to be completely consistent and bug-free. The purpose of release branches is to ensure this.
Anyway, back to the actual changes. I like the cleanness of the code, but I would prefer more docstrings and comments to explain why things are the way they are. I also noticed a bug.
I created a new collection with the slug extra through the browsable API. The URI of this collection is http://localhost:8000/rdf/collections/extra. Create, retrieve, update and delete all work. However:
- In the triplestore, the slug of the associated project is duplicated. Its URI is
http://localhost:8000/rdf/project/Dev/Deveverywhere (so at least the URI is consistent). Note thatprojectis singular as well. By contrast, my project that was migrated from a group has a pluralprojectsand contains the slug only once:http://localhost:8000/rdf/projects/dev_g. - I updated my newly created collection to associate it with the
dev_gproject instead. However, the corresponding object node in the triplestore now held the URIhttp://localhost:8000/rdf/project/dev_g/dev_g.
Some more details in the comments. Keep up the sweet refactoring!
|
|
||
| # try to create the same collection again | ||
| fail_response = post_collection(client, project.name) | ||
| assert is_client_error(fail_response.status_code) |
There was a problem hiding this comment.
Once database synchronization is added, I would also want to test here that there is no duplicate entry in the database.
There was a problem hiding this comment.
My impression was that if you add triples that already exist in blazegraph (in the same graph), it will have no effect. That is, the same triple won't be stored twice. Since the request to make a collection is idempotent, and this test makes the same request twice, I don't think you could check if the second time was executed?
What does make sense to me is to create a different collection in the second request (e.g. with a different description), to check that the create request doesn't store the new data. (This scenario is also why the api should reject the request.)
Correct me if I'm wrong about blazegraph here, though!
There was a problem hiding this comment.
You are right about Blazegraph, but I meant the representation of the collection in the PostgreSQL database. Hence "once database synchronization is added".
| ('Test!!', 'test'), | ||
| ('Test test test', 'test_test_test'), |
There was a problem hiding this comment.
These test cases are quite conservative. Add some Bobby Tables and some chimpanzee speak as well.
| from triplestore.utils import Quads | ||
|
|
||
|
|
||
| class CollectionsField(RDFQuadField): |
There was a problem hiding this comment.
Please add a docstring to explain the purpose of this class.
| def get(self, instance: RDFModel): | ||
| return [ | ||
| s | ||
| for (s, p, o, g) in self._stored_quads(instance) | ||
| ] | ||
|
|
||
| def _stored_quads(self, instance: RDFModel) -> Quads: | ||
| store = settings.RDFLIB_STORE | ||
| results = store.query(f''' | ||
| SELECT ?col WHERE {{ | ||
| ?col a edpopcol:Collection ; | ||
| as:context <{instance.uri}> . | ||
| }} | ||
| ''', initNs={'as': AS, 'edpopcol': EDPOPCOL}) | ||
|
|
||
| return [ | ||
| (result, AS.context, instance.uri, Graph(store, result)) | ||
| for (result, ) in results | ||
| ] |
There was a problem hiding this comment.
I am not sure, but it looks to me as if the query in _stored_quads gets the information that get is looking for. However, _stored_quads decorates this information with additional data, which get then peels off again. Wouldn't it be more straightforward and efficient to put the query in get and then call get from _stored_quads?
There was a problem hiding this comment.
I get the confusion. The reason is that _stored_quads is also used to update the triplestore. When you call save() on the model, the field will compare _stored_quads() with _quads_to_store() to update the graph. _stored_quads() is also used when you call delete().
So _stored_quads() returns the representation that is saved in the triplestore, which is needed to make updates. get() translates the graph representation to one that is more convenient in the model.
This is implemented on the parent class (RDFQuadField), but it looks convoluted in isolation.
There was a problem hiding this comment.
Sure, but you are in full control here because you are overriding both methods. What stops you from writing this?
| def get(self, instance: RDFModel): | |
| return [ | |
| s | |
| for (s, p, o, g) in self._stored_quads(instance) | |
| ] | |
| def _stored_quads(self, instance: RDFModel) -> Quads: | |
| store = settings.RDFLIB_STORE | |
| results = store.query(f''' | |
| SELECT ?col WHERE {{ | |
| ?col a edpopcol:Collection ; | |
| as:context <{instance.uri}> . | |
| }} | |
| ''', initNs={'as': AS, 'edpopcol': EDPOPCOL}) | |
| return [ | |
| (result, AS.context, instance.uri, Graph(store, result)) | |
| for (result, ) in results | |
| ] | |
| def get(self, instance: RDFModel): | |
| store = settings.RDFLIB_STORE | |
| results = store.query(f''' | |
| SELECT ?col WHERE {{ | |
| ?col a edpopcol:Collection ; | |
| as:context <{instance.uri}> . | |
| }} | |
| ''', initNs={'as': AS, 'edpopcol': EDPOPCOL}) | |
| return [ | |
| s | |
| for (s, ) in results | |
| ] | |
| def _stored_quads(self, instance: RDFModel) -> Quads: | |
| store = settings.RDFLIB_STORE | |
| return [ | |
| (result, AS.context, instance.uri, Graph(store, result)) | |
| for result in self.get(instance) | |
| ] |
There was a problem hiding this comment.
Ah right. Nothing, but to explain why it wasn't written like that: in general, _stored_triples/_stored_quads may contain information that is ignored by the model (and thus not retrievable from the output of get()).
For instance, you could write this field to be agnostic about the graph context. In that case, _stored_quads should include the graph the as:context triple was stored in, because that information is necessary for save/delete operations. But get() can filter it out.
So in general, it makes sense to write _stored_triples/_stored_quads first, and then convert that to an internal value in get(). In this specific case, though, you're right that the dependency can flow either way.
This adds an API for collections which are saved in the triplestore. close #159, close #160
It adds list, retrieve, create, update, and delete endpoints for collections. (Which replace the existing API for collections.)
This is implemented as a "regular" JSON API, but collections are identified by an URI instead of an integer.
This update only implements the API on the backend, so it will break the frontend (which expects a slightly different API for collections). I expect adjusting the frontend will go smoothly, but I did not want to bloat this PR.
In the meantime, you can test the browsable API at
/api/collections/.This update also does not include a data migration or adjust the existing data migration script; I'll add that in a later PR.
Other changes:
vreapp to the project moduleedpop.