-
Notifications
You must be signed in to change notification settings - Fork 0
Generic entity endpoint #290
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
base: main
Are you sure you want to change the base?
Conversation
app/routers/entity.py
Outdated
db: SessionDep, | ||
user_context: UserContextDep, | ||
): | ||
return get_readable_entity( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use router_read_one here from app/queries/common.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation changed a bit, but for this endpoint the caller doesn't necessarily know the project_id so we have to retrieve the entity from the db, and only then check if they have auth, we might as well return it here instead of doing an extra db call through "router_read_one".
app/routers/entity.py
Outdated
id_: UUID, | ||
db: SessionDep, | ||
user_context: UserContextDep, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add also the returned type annotation?
Is it ok to return a generic entity, instead of the specific class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added EntityRead as return type.
Yes, that is the intent, the frontend will redirect to the specific page based on the type.
@@ -16,7 +16,7 @@ class NestedEntityRead(BaseModel): | |||
|
|||
|
|||
class EntityRead(NestedEntityRead, CreatedByUpdatedByMixin): | |||
pass | |||
virtual_lab_id: uuid.UUID | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand why the virtual_lab_id should be part of the EntityRead model returned by entitycore? entitycore uses the virtual_lab_id for determining the S3 key of the assets, but it's not part of other models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for this feature the caller might only know the id of the entity, but not the virtual_lab_id
or project_id
.
But the FE needs to know the virtual_lab_id to be able to redirect the user to the correct page, for example:
/app/virtual-lab/lab/{vlab_id}/project/{project_id}/explore/interactive/experimental/{entity-type}/{id}
Details here: https://github.com/openbraininstitute/prod-explore-functionality/issues/245#issuecomment-3079059771
Maybe we can create a separate schema EntityReadWithVlabId for this endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From that ticket:
If private we check credentials first then return vlab-id and project id and redirect to /app/virtual-lab/lab/{vlab_id}/project/{project_id}/explore/interactive/experimental/{entity-type}/{id}
But if the entity is private, the frontend already knows the vlab_id and project_id, so why does it need to get it from entitycore?
Cannot the logic based on the authorized_public
flag?
- if it's public:
/app/explore/{entity-type}/{id}
- if it's private:
/app/virtual-lab/lab/{vlab_id}/project/{project_id}/explore/interactive/experimental/{entity-type}/{id}
but use the same vlab_id and project_id that were set in the request headers to entitycore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my original proposal, yes. But then jdc requested that that url be generic:
/entity-detail/id
The frontend doesn't know the virtual-lab-id from that url.
Required for:
https://github.com/openbraininstitute/prod-explore-functionality/issues/245