Backend/feature/5123 update community #5373
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
jasonwvh
changed the title
There was a problem hiding this comment.
Thank you for working on this! I think the update community RPC is looking great and is mostly good to merge with some minor comments.
Sorry for taking so long to get to this, I've been in Oz and busy with family stuff.
I've added some general comments on the code otherwise too.
On the deletion stuff: I'm not entirely sure how we should handle the cases of dependent objects when their creator/owner/parent is deleted.
For example the main page of a community (cluster) is a designated page: if one (the cluster or the page) is deleted, the other should be too. Maybe this is easy by saying main pages cannot be deleted with a CHECK constraint (so deleted cluster => deleted page).
But what about when a cluster is deleted, all its sub-pages should be deleted too, it seems?
Or if a community is deleted, all sub-communities should also be deleted...
We will need to think about it. I guess there are three ways to go:
- Make only some things soft-deleteable (e.g. non-main pages) and hard-delete other things (nodes, clusters): I feel like having soft-deleted nodes would be a pain in the ass code-wise.
- Figure out some way to enforce the deletion hierarchy via database constraints or similar. Not sure how to do this, but this would be the cleanest.
- Enforce deletion hierarchy via our code, or via TRIGGERs. This is my least preferred solution since it's prone to leaving the database in a broken state, but if we can't think of other things, we might go with this.
app/backend/src/couchers/models.py
Outdated
| @@ -1513,6 +1513,7 @@ class Node(Base): | |||
| parent_node_id = Column(ForeignKey("nodes.id"), nullable=True, index=True) | |||
| geom = deferred(Column(Geometry(geometry_type="MULTIPOLYGON", srid=4326), nullable=False)) | |||
| created = Column(DateTime(timezone=True), nullable=False, server_default=func.now()) | |||
| deleted = Column(DateTime(timezone=True), nullable=True, default=None) | |||
There was a problem hiding this comment.
No need for default since it's nullable
| new_node = Node(geom=geom, parent_node_id=parent_node_id) | ||
| session.add(new_node) | ||
|
|
||
| now = datetime.utcnow() |
There was a problem hiding this comment.
Please use now() from utils.py (to be consistent, also I'm not sure if utcnow (which is naive, I believe) works correctly if the server TZ is not UTC)
| session.add(new_node) | ||
|
|
||
| now = datetime.utcnow() | ||
| session.execute(update(Node).where(Node.id == node.id).values({"deleted": now})) |
There was a problem hiding this comment.
It's better to do this through the ORM, you can simply assign to node.deleted, so this would be
node.deleted = now()session.execute will actually issue a SQL statement here. Using the ORM way will issue them all at flush or commit (which happens automatically as well at the end of the RPC due to the session_scope context manager).
This applies to a bunch of places here. We only really need a low-level UPDATE when updating a lot of things (where it's undesirable to have SQLAlchemy load everything into python objects), or when updating based on a computed SQL expression we want rendered directly (e.g. UPDATE table SET somevalue = othervalue + 2... or similar).
I am also not entirely sure how this interacts with the SQLAlchemy ORM: it might cause a bunch of refreshing objects (extra SELECTs), or it might cause the object to be stale in other ways.
| @@ -283,6 +285,7 @@ def ListPlaces(self, request, context, session): | |||
| places = ( | |||
| node.official_cluster.owned_pages.where(Page.type == PageType.place) | |||
| .where(Page.id >= next_page_id) | |||
| .where(Page.deleted is None) | |||
There was a problem hiding this comment.
Compare to None with deleted == None. Or I think the actual right way to do this is to do Page.deleted.is_(None), but == None works too. I believe is None might cause issues.
This is weird minutiae of SQLalchemy ORM mapping.
…te-community' into backend/feature/5123-update-delete-community
|
@aapeliv seems like the |
jasonwvh
changed the title
| cluster = session.execute(select(Cluster).where(Cluster.id == request.community_id)).scalar_one_or_none() | ||
| if not cluster: | ||
| context.abort(grpc.StatusCode.NOT_FOUND, errors.COMMUNITY_NOT_FOUND) | ||
|
|
||
| node = session.execute(select(Node).where(Node.id == cluster.parent_node_id)).scalar_one_or_none() | ||
| if not node: | ||
| context.abort(grpc.StatusCode.NOT_FOUND, errors.COMMUNITY_NOT_FOUND) |
There was a problem hiding this comment.
The community_id corresponds to node_id; so you ought to do
node = session.execute(select(Node).where(Node.id == request.community_id)).scalar_one_or_none()
if not node:
context.abort(grpc.StatusCode.NOT_FOUND, errors.COMMUNITY_NOT_FOUND)
cluster = node.official_clusterThere was a problem hiding this comment.
Got it, so community_id is actually node_id not cluster_id
| geom = shape(json.loads(request.geojson)) | ||
| if geom.geom_type != "MultiPolygon": | ||
| context.abort(grpc.StatusCode.INVALID_ARGUMENT, errors.NO_MULTIPOLYGON) | ||
| node.geom = from_shape(geom) |
There was a problem hiding this comment.
Could you please refactor this into a function, something like load_community_geom(geojson, context), then use it as well in CreateCommunity?
Database migrations:deletedtimestamp column forclusters,node,pages,page_versionsNew endpoints:
community_idand updatesname,description,parent_node_id, andgeojsonadmin_idshere?DeleteCommunitysetsdeletedtonow()inclusters,node,pages,page_versionspartially closes #5123
Backend checklist
ruff check --select I --fix . && ruff check . && ruff format .inapp/backenddevelopif necessary for linear migration historyWeb frontend checklist
yarn formatyarn lint --fix