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
Fix ISE on entity pages involving MBID redirects #145
Conversation
@@ -47,8 +48,11 @@ def get_entities_by_gids(*, query, entity_type, mbids): | |||
redirects = query.session.query(redirect_model).filter(redirect_model.gid.in_(remaining_gids)) | |||
redirect_gids = {redirect.redirect_id: redirect.gid for redirect in redirects} | |||
redirected_entities = query.filter(redirect_model.redirect.property.primaryjoin.left.in_(redirect_gids.keys())).all() | |||
for entity in redirected_entities: | |||
entity.gid = redirect_gids[entity.id] |
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.
Because these are ORM objects, I wonder if there is a better way of doing this operation so that we don't modify them. What about returning a dictionary of {gid: entity}
where the gid
could be a redirect gid, and not just entity.gid
. This is quite similar to what you're doing in fetch_multiple_x
anyway, and means that we're not passing around modified but not committed ORM objects.
This now also solves CB-130 (for frontend).
So, pages |
@@ -37,20 +37,19 @@ def get_entities_by_gids(*, query, entity_type, mbids): | |||
mbids (list): IDs of the target entities. | |||
|
|||
Returns: | |||
List of objects of target entities. | |||
Dictionary of objects of target entities keyed by their mbid. |
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.
MBID
Due to sqlalchemy's
autoflushing
, it issues anupdate
query while modifying redirected entity'sgid
field and this issue is raised https://sentry.metabrainz.org/share/issue/342e32363535/. I've updated the functionget_entities_by_gids
for fixing this. Eg. https://critiquebrainz.org/release-group/820ba931-840d-45f1-97ed-0cceaff26da0