Validate image existence before post or preview #998
Conversation
paskal
force-pushed
the
dverhoturov/validate_image_before_post
branch
2 times, most recently
from
241b50c
to
8aba64d
Compare
Pull Request Test Coverage Report for Build 845751851
💛 - Coveralls |
paskal
force-pushed
the
dverhoturov/validate_image_before_post
branch
3 times, most recently
from
b20ca23
to
be7fe0f
Compare
paskal
force-pushed
the
dverhoturov/validate_image_before_post
branch
from
be7fe0f
to
f00d433
Compare
|
@umputun now it's properly working both in reality and tests, please review and merge if everything is OK. |
backend/app/store/image/image.go
Outdated
| return | ||
| } | ||
| ids = append(ids, imgID) | ||
| } | ||
| } | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
I'm not sure if the error check here safe/correct. I mean, extraction before this change worked even for partially valid document and returned some images anyway. I think it was intentional, and now we may have all or nothing
There was a problem hiding this comment.
Now it's all or nothing, but there is no way you get an error on submitImages (the only function using that function before the PR) after comment sending now because post sending calls the same function and won't allow posting incorrect images.
I don't recall an intention of ignoring the error when I wrote cache image extraction, when I was preparing that PR I was surprised by the bare return inside that function returning null instead of err
There was a problem hiding this comment.
hmm, the function ExtractPictures returned an error only in one case - goquery.NewDocumentFromReader failed. Everything else was forgiving and func tried to extract as many pictures as possible from the comment. After this change instead of valid pictures, submitImages won't get anything if one of the images failed, and idsFn will return an empty slice. Are you sure this behavior valid?
Is this change even relevant to the PR? I don't see how changing the logic of ExtractPictures is necessary for the proper validation.
There was a problem hiding this comment.
Without that change, we do not validate cached images. But they are available in the comment only on edit and not on the post and we don't validate edits in this PR.
I think you are right and it would be better to revert that function change and document it better instead.
There was a problem hiding this comment.
Now, as we talk about it, I started wondering if we should return the error at all: it's confusing to have an error of HTML parsing returned to the caller but cached image parsing errors hidden in the code. Making it errorless will clarify the intention of returning ids at any cost, what do you think?
There was a problem hiding this comment.
the error it returns now is kind of "panic case, should never happen". Logging this thing as we do is ok strategy, just in case.
paskal
force-pushed
the
dverhoturov/validate_image_before_post
branch
from
f00d433
to
32493d9
Compare
That should cover the case when an image is posted after it cleaned up or the URL is altered by mistake: in case image belongs to this site, it should be working at the time of preview or comment post.