From bdc9bfbc78c3dffc989a0c0e456bf1b2ead4732c Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Mon, 17 May 2021 02:29:49 +0200 Subject: [PATCH] remove error return from ExtractPictures That function returns an error in a condition that is never expected, and the end result of that error would be logged message on the caller side: none of the callers handles it. That change hides that error from the caller so that function would have signature which better fit what it does and how it behaves. --- backend/app/rest/api/rest_private.go | 9 ++------- backend/app/rest/api/rest_public.go | 7 +------ backend/app/store/image/image.go | 9 ++++----- backend/app/store/image/image_test.go | 18 ++++++------------ backend/app/store/service/service.go | 6 +----- 5 files changed, 14 insertions(+), 35 deletions(-) diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 3db49cbb3e..b9dd0015f7 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -89,13 +89,8 @@ func (s *private) createCommentCtrl(w http.ResponseWriter, r *http.Request) { comment = s.commentFormatter.Format(comment) // check if images are valid - imgIds, err := s.imageService.ExtractPictures(comment.Text) - if err != nil { - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't extract pictures from comment text", rest.ErrCommentValidation) - return - } - for _, id := range imgIds { - _, err = s.imageService.Load(id) + for _, id := range s.imageService.ExtractPictures(comment.Text) { + _, err := s.imageService.Load(id) if err != nil { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't load picture from the comment", rest.ErrImgNotFound) return diff --git a/backend/app/rest/api/rest_public.go b/backend/app/rest/api/rest_public.go index f6c7efb0d0..e1dabc896b 100644 --- a/backend/app/rest/api/rest_public.go +++ b/backend/app/rest/api/rest_public.go @@ -134,12 +134,7 @@ func (s *public) previewCommentCtrl(w http.ResponseWriter, r *http.Request) { comment.Sanitize() // check if images are valid - imgIds, err := s.imageService.ExtractPictures(comment.Text) - if err != nil { - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't extract pictures from comment text", rest.ErrCommentValidation) - return - } - for _, id := range imgIds { + for _, id := range s.imageService.ExtractPictures(comment.Text) { _, err = s.imageService.Load(id) if err != nil { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't load picture from the comment", rest.ErrImgNotFound) diff --git a/backend/app/store/image/image.go b/backend/app/store/image/image.go index e61ce248b6..30f6113d7f 100644 --- a/backend/app/store/image/image.go +++ b/backend/app/store/image/image.go @@ -148,13 +148,12 @@ func (s *Service) Submit(idsFn func() []string) { } // ExtractPictures gets list of images from the doc html and convert from urls to ids, i.e. user/pic.png -// It doesn't return possible errors parsing the cached images, and will try to return as many parsed ids -// as possible instead. -func (s *Service) ExtractPictures(commentHTML string) (ids []string, err error) { +func (s *Service) ExtractPictures(commentHTML string) (ids []string) { doc, err := goquery.NewDocumentFromReader(strings.NewReader(commentHTML)) if err != nil { - return nil, errors.Wrap(err, "can't create document") + log.Printf("[ERROR] can't parse commentHTML to parse images: %q, error: %v", commentHTML, err) + return nil } doc.Find("img").Each(func(i int, sl *goquery.Selection) { if im, ok := sl.Attr("src"); ok { @@ -183,7 +182,7 @@ func (s *Service) ExtractPictures(commentHTML string) (ids []string, err error) } }) - return ids, nil + return ids } // Cleanup runs periodic cleanup with cleanupTTL. Blocking loop, should be called inside of goroutine by consumer diff --git a/backend/app/store/image/image_test.go b/backend/app/store/image/image_test.go index 04e23790fd..c04628bea6 100644 --- a/backend/app/store/image/image_test.go +++ b/backend/app/store/image/image_test.go @@ -77,8 +77,7 @@ func TestService_ExtractPictures(t *testing.T) { svc := Service{ServiceParams: ServiceParams{ImageAPI: "/blah/", ProxyAPI: "/non_existent"}} html := `blah foo xyz

123

` - ids, err := svc.ExtractPictures(html) - require.NoError(t, err) + ids := svc.ExtractPictures(html) require.Equal(t, 2, len(ids), "two images") assert.Equal(t, "user1/pic1.png", ids[0]) assert.Equal(t, "user2/pic3.png", ids[1]) @@ -90,35 +89,30 @@ func TestService_ExtractPictures(t *testing.T) {

bjtr0-201906-08110846-i324c.png

\n\n

По форме все верно, это все packages, но по сути это все одна библиотека организованная таким образом. При ее импорте, например посредством go mod, она выглядит как один модуль, т.е. github.com/go-pkgz/auth v0.5.2.

\n` - ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + ids = svc.ExtractPictures(html) require.Equal(t, 1, len(ids), "one image in") assert.Equal(t, "github_ef0f706a79cc24b17bbbb374cd234a691d034128/bjttt8ahajfmrhsula10.png", ids[0]) // proxied image html = `cat.png` - ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + ids = svc.ExtractPictures(html) require.Equal(t, 1, len(ids), "one image in") assert.Equal(t, "cached_images/12318fbd4c55e9d177b8b5ae197bc89c5afd8e07-a41fcb00643f28d700504256ec81cbf2e1aac53e", ids[0]) // bad url html = `` - ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + ids = svc.ExtractPictures(html) require.Empty(t, ids) // bad src html = `` - ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + ids = svc.ExtractPictures(html) require.Empty(t, ids) // good src with bad content badURL := base64.URLEncoding.EncodeToString([]byte(" http://foo.bar")) html = fmt.Sprintf(``, badURL) - ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + ids = svc.ExtractPictures(html) require.Empty(t, ids) } diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go index 96957f8630..9fdba2ff45 100644 --- a/backend/app/store/service/service.go +++ b/backend/app/store/service/service.go @@ -235,11 +235,7 @@ func (s *DataStore) submitImages(comment store.Comment) { log.Printf("[WARN] can't get comment's %s text for image extraction, %v", comment.ID, err) return nil } - imgIds, err := s.ImageService.ExtractPictures(cc.Text) - if err != nil { - log.Printf("[WARN] can't get extract pictures from %s, %v", comment.ID, err) - return nil - } + imgIds := s.ImageService.ExtractPictures(cc.Text) if len(imgIds) > 0 { log.Printf("[DEBUG] image ids extracted from %s - %+v", comment.ID, imgIds) }