Skip to content
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

Merged
merged 3 commits into from Aug 11, 2017

Conversation

ferbncode
Copy link
Member

@ferbncode ferbncode commented Aug 9, 2017

Due to sqlalchemy's autoflushing, it issues an update query while modifying redirected entity's gid field and this issue is raised https://sentry.metabrainz.org/share/issue/342e32363535/. I've updated the function get_entities_by_gids for fixing this. Eg. https://critiquebrainz.org/release-group/820ba931-840d-45f1-97ed-0cceaff26da0

@@ -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]
Copy link
Collaborator

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.

@ferbncode
Copy link
Member Author

ferbncode commented Aug 10, 2017

This now also solves CB-130 (for frontend).

  • For eg. /release-group/820ba931-840d-45f1-97ed-0cceaff26da0 will show reviews for /release-group/bf63ed7c-c15f-4e5d-8711-c495233b7cff (Original MBID).
  • Also, adding a review for the release-group at /release-group/820ba931-840d-45f1-97ed-0cceaff26da0 will add a review with entity_id: bf63ed7c-c15f-4e5d-8711-c495233b7cff.
  • Other actions like deleting a review, updating a review will also be the same.

So, pages /release-group/820ba931-840d-45f1-97ed-0cceaff26da0 (additional MBID) and /release-group/bf63ed7c-c15f-4e5d-8711-c495233b7cff (original MBID) will be completely same. This was the behavior intended for solving CB-130. right?. Please suggest.

@gentlecat gentlecat changed the title Fix ISE on entity pages involving mbid redirects. Fix ISE on entity pages involving MBID redirects Aug 11, 2017
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MBID

@gentlecat gentlecat merged commit 36111c8 into metabrainz:master Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants