Repository navigation
Finish round 3 singletons: S8519, S112, ratings S3776 (Sonar) - #562
Merged
Merged
Conversation
- amo.search.ES.__getitem__ (python:S8519): _first_item() takes the first result without list(); it still raises IndexError on an empty result. Same queries: _do_search caches, so one ES query, and for object results the queryset is evaluated once either way. - amo.utils.resize_image and lib.settings_base.read_only_mode (python:S112): ValueError and RuntimeError instead of bare Exception, same messages. Every caller that catches it uses except Exception, so it is still caught. - ratings.views.RatingViewSet.filter_queryset (python:S3776, still 18 after #535): the list-action body moves verbatim into _filter_list_queryset(). Refs #419
2 tasks
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Round 3 remainder for thunderbird/addons-server#419, as listed in the coordinator's status note: S8519, the two S112s, and the ratings S3776 still over 15 after #535.
Changes, each with its no-behavior-change argument
src/olympia/amo/search.pyES.__getitem__(AaEh92RwMSBE_HpBvXx8(line 104)).return list(new)[0]becomesreturn _first_item(new). This is the_first_item()pattern from P1-11: Sonar fixes in activity (S8512, S8519, S7496, S5806, S1192, S2638, S1172, S1940) #473:next(iter(...)), re-raisingIndexError('list index out of range')on an empty result, the same type and messagelist(...)[0]raised.list(new)calledES.__len__and thenES.__iter__. Both go through_do_search(), which caches_results_cache, so that was one ES query.next(iter(new))calls__iter__->_do_search()once: still one ES query.len()evaluated thetype.objects.filter(id__in=...)queryset, andObjectSearchResults.__iter__then reused its cache. Now__iter__evaluates it, still exactly once. That generator builds its dict eagerly, so taking only the first element skips no work.src/olympia/amo/utils.pyresize_image(AaEh92NnMSBE_HpBvXxT(line 668)):ExceptionbecomesValueError, with the same message. Callers:devhub/tasks.py(icon/preview resizing) andusers/tasks.pywrap it inexcept Exception, which still catchesValueError.versions/tasks.pyand the other devhub calls don't catch it, so it propagates as before.amo/tests/test_amo_utils.py::test_resize_imageusespytest.raises(Exception, match=...), which still passes.src/olympia/lib/settings_base.pyread_only_mode(AaEh92ixMSBE_HpBvX1X(line 1451)):ExceptionbecomesRuntimeError, with the same message. No caller catches it: it runs at settings import (oramo/tests/test_readonly.py's fixture with a valid env). I used a built-in so the settings module doesn't gain a Django import.src/olympia/ratings/views.pyRatingViewSet.filter_queryset(AaEh92f7MSBE_HpBvX0l(line 373)): the wholeif self.action == 'list':body moves verbatim into_filter_list_queryset(qs), which returnsqs. The same parameters are read and parsed in the same order, with the sameParseErrormessages and the sameself.pagination_classassignment.filter_querysetstill callssuper().filter_queryset(qs)last.Decorators checked: the AST comparison of every pre-existing function's decorator list between stage-eks and this branch shows 0 changes.
RatingViewSet.filter_querysetandES.__getitem__carry no decorators, before or after;_first_itemand_filter_list_querysetare new and undecorated.Tests
flake8 is clean on changed lines. CI pytest covers
amo/tests/test_search.py,amo/tests/test_amo_utils.py,amo/tests/test_readonly.pyandratings/tests/test_views.py. Local Greptile review skipped: org quota.