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.

Sign up for GitHub

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 editing multi-build updates when title failed to update #2435

Merged
merged 1 commit into from
Jul 26, 2018
Merged

Fix editing multi-build updates when title failed to update #2435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions bodhi/server/validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,13 +258,11 @@ def validate_builds(request, **kwargs):
'Cannot edit stable updates')

for nvr in request.validated.get('builds', []):
# If the build is new
if nvr not in edited:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this was here just to help with performance? It seems like it was just trying to make this verification when an NVR was being added to an update. Do you think we need to always perform this test?

# Ensure it doesn't already exist
build = request.db.query(Build).filter_by(nvr=nvr).first()
if build and build.update is not None:
request.errors.add('body', 'builds',
"Update for {} already exists".format(nvr))
# Ensure it doesn't already exist in another update
build = request.db.query(Build).filter_by(nvr=nvr).first()
if build and build.update is not None and up.title != build.update.title:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention here to compare up to build.update? If so, it might be good to compare the alias field instead of the title field since we want to remove/repurpose the title field some day.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm not sure, but you might be able to use the is operator to compare them. I'd have to read some sqlalchemy docs to verify. If it were me, I'd just play it safe and compare a field I know to be unique like you did here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll switch to the .alias field.
However, the whole problem is that sometimes the update's title doesn't get correctly updated, and on later update, if one of the builds in the update remain, it will abort otherwise.

My example from the test:

  • I submitted a-1.0-1.fc17 b-1.0-1.fc17 c-1.0-1.fc17
  • At some point, I edited that to replace b-1.0-1.fc17 with b-1.0-2.fc17. However, due to a sometimes-occuring bug, the update.title is still a-1.0-1.fc17 b-1.0-1.fc17 c-1.0-1.fc17. This has happened a few times, and given that we're intending to remove/repurpose title, I didn't feel like fixing that.
  • I then want to edit it to replace c-1.0-1.fc17 with c-2.0-1.fc17. At this point, Bodhi checks each of the new builds whether they are in an existing update that is not the update I'm testing. It does that by testing against the nvr in update.title. HOWEVER, since the title didn't update correctly after the first edit, it no longer sees b-1.0-2.fc17 as part of this update, but when it goes request that build, that build does have an update attached to it.
  • My last edit now fails, because b-1.0-2.fc17 is in another update.

This patch fixes that problem by instead of checking whether all of the new builds are listed in the update.title, just look if the update the build is in is the one we're currently editing, thus fixing the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, thanks for the detailed explanation!

request.errors.add('body', 'builds',
"Update for {} already exists".format(nvr))

return

Expand Down
54 changes: 54 additions & 0 deletions bodhi/tests/server/services/test_updates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2630,6 +2630,60 @@ def test_edit_testing_update_with_new_builds(self, publish, *args):
self.assertEquals(len(publish.call_args_list), 3)
publish.assert_called_with(topic='update.edit', msg=ANY)

@mock.patch(**mock_valid_requirements)
@mock.patch('bodhi.server.notifications.publish')
def test_edit_testing_update_with_failed_title_change(self, publish, *args):
"""Test that editing a multi-build update is still possible if title updating failed.

At some point, the text of the title was used to determine whether a build was
in the current update.
"""
nvrs = 'a-1.0-1.fc17,b-1.0-1.fc17,c-1.0-1.fc17'
args = self.get_update(nvrs)
r = self.app.post_json('/updates/', args)
publish.assert_called_with(topic='update.request.testing', msg=ANY)

# Mark it as testing
upd = Update.get(r.json_body['alias'])
newtitle = upd.title.replace('a-1.0', 'a-1.5')
upd.title = newtitle
upd.status = UpdateStatus.testing
upd.request = None
self.db.commit()

args['edited'] = newtitle
# Kept a and c the same, bumped b
args['builds'] = ['a-1.0-1.fc17', 'b-2.0-1.fc17', 'c-1.0-1.fc17']
r = self.app.post_json('/updates/', args)
up = r.json_body
self.assertEquals(up['title'], u'a-1.0-1.fc17 b-2.0-1.fc17 c-1.0-1.fc17')
Copy link
Contributor

Choose a reason for hiding this comment

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

The name and description of the test makes it sound like we expect the title not to change, but it seems like the title is correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This catches the case where the title didn't correctly update on an earlier update, read my summary from the other comment.

self.assertEquals(up['status'], u'pending')
self.assertEquals(up['request'], u'testing')
self.assertEquals(up['comments'][-1]['text'],
u'This update has been submitted for testing by guest. ')
comment = textwrap.dedent("""
guest edited this update.

New build(s):

- b-2.0-1.fc17

Removed build(s):

- b-1.0-1.fc17
Copy link
Contributor

Choose a reason for hiding this comment

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

One day I will add tags to the Comment model so we don't have to have Bodhi test its own comments to see if stuff like this happened :)


Karma has been reset.
""").strip()
self.assertMultiLineEqual(up['comments'][-2]['text'], comment)
self.assertEquals(up['comments'][-3]['text'],
u'This update has been submitted for testing by guest. ')
self.assertEquals(len(up['builds']), 3)
self.assertEquals(up['builds'][0]['nvr'], u'a-1.0-1.fc17')
self.assertEquals(up['builds'][1]['nvr'], u'c-1.0-1.fc17')
self.assertEquals(up['builds'][2]['nvr'], u'b-2.0-1.fc17')
self.assertEquals(len(publish.call_args_list), 3)
publish.assert_called_with(topic='update.edit', msg=ANY)

@mock.patch(**mock_valid_requirements)
@mock.patch('bodhi.server.notifications.publish')
def test_edit_testing_update_with_new_builds_with_stable_request(self, publish, *args):
Expand Down