From 4d011aa5f3f484c663ec0bfea908a87acad4a012 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 never expected condition, and 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 a signature that 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 13ec3c0f38..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 b61e37e54e..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 7b7ed23c9b..96d2c04691 100644 --- a/backend/app/store/image/image.go +++ b/backend/app/store/image/image.go @@ -137,13 +137,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 { @@ -172,7 +171,7 @@ func (s *Service) ExtractPictures(commentHTML string) (ids []string, err error) } }) - return ids, nil + return ids } // Cleanup runs periodic cleanup with 2.5*ServiceParams.EditDuration. 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 4158945075..229819f4c7 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 a2b98548bf..19c8e4b156 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) }