Skip to content

Add ability to specify URL for an image #587

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Add ability to specify URL for an image #587

wants to merge 6 commits into from

Conversation

php-coder
Copy link
Owner

@php-coder php-coder commented May 17, 2017 •
edited
Loading

TODO:

  • fix integration tests
  • fix errors from static analysis tools
  • show errors near the URL field
  • show errors that has happened during file downloading
  • add title to field that only HTTP protocol is supported and no redirects are allowed
  • MyMultipartFile: preserve original content type
  • fix 2 error messages for invalid protocol
  • add localization for errors from interceptor
  • add localization for @URL validator
  • validation: image or imageUrl must be specified
  • validation: image and imageUrl cannot be specified at the same time
  • DownloadImageInterceptor: make URLs and fields configurable
  • extract downloader to a service
  • implement for /series/{id} page
  • Handle HttpURLConnection.HTTP_MOVED_TEMP || status == HttpURLConnection.HTTP_MOVED_PERM separately
  • invoke setConnectTimeout() ?
  • add acceptance tests
    • user specifies image as URL (http://127.0.0.1:8080/image/1)
    • user specifies image as URL to an empty file (error expected)
    • user specifies image as URL to a file of unsupported type (error expected)
    • user specifies image as URL to a file that doesn't exist (error expected)
    • user specifies image as URL that causes redirect (error expected)
    • user specifies image as URL but the file couldn't be downloaded (error expected)
  • replace togglz by authority
  • fix validation: when user doesn't have perms he see the error "Image or image URL must be specified" instead of "Required field"
  • update changelog
  • add a test: user should have required image field with error "Required field"
  • add a test: admin should have required image and image url fields with error "Image or image URL must be specified"
  • consider what to do with @NotEmptyFilename validator

Addressed to #199

@php-coder
Copy link
Owner Author

@AleksSPb @Shkarin @Shagbark In case you're interesting to review yet another my PR. It's a working draft that will be improved. Right now I would like to get an early feedback and ideas. There are also some security concerns that we could discuss (here or in the mailing list).

@php-coder php-coder force-pushed the download_image branch 2 times, most recently from 5aae4ba to f2099c6 Compare May 18, 2017 22:02
asm0dey
asm0dey reviewed May 19, 2017
View reviewed changes
src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated

@Override
@SuppressWarnings("PMD.SignatureDeclareThrowsException")
public boolean preHandle(
Copy link
Contributor

Choose a reason for hiding this comment

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

Я дико извиняюсь, но размер этого метода абсолютно невменяем и что он делает тоже непонятно.

src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated

// TODO: add title to field that only HTTP protocol is supported
// and no redirects are allowed
if (!"http".equals(url.getProtocol())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we suddenly use https?

src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated
LOG.debug("getReadTimeout(): {}", conn.getReadTimeout());

// TODO: how bad is it?
conn.setInstanceFollowRedirects(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's very bad. We know nothing about http server. It could easily redirect us to https, for example.

src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated

try (InputStream stream = new BufferedInputStream(conn.getInputStream())) {
int status = conn.getResponseCode();
if (status != HttpURLConnection.HTTP_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we've got another 20x status?

src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated

String contentType = conn.getContentType();
LOG.debug("Content-Type: {}", contentType);
if (!"image/jpeg".equals(contentType) && !"image/png".equals(contentType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

svg? gif?

src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated
}

} catch (IOException ex) {
LOG.error("Couldn't open connection: {}", ex.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Here could be lots of errors — e.g. readtimeout, which actually isn't problem in connection opening

src/main/java/ru/mystamps/web/controller/interceptor/DownloadImageInterceptor.java Outdated
// TODO: preserve original content type
@Override
public String getContentType() {
return "image/jpeg";
Copy link
Contributor

Choose a reason for hiding this comment

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

???

src/main/java/ru/mystamps/web/model/AddSeriesForm.java Outdated
@@ -128,13 +132,17 @@
@Size(max = MAX_SERIES_COMMENT_LENGTH, message = "{value.too-long}")
private String comment;

@NotNull
// @NotNull
Copy link
Contributor

Choose a reason for hiding this comment

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

commented-out code?

src/main/java/ru/mystamps/web/model/AddSeriesForm.java Outdated
@NotEmptyFilename(groups = Image1Checks.class)
@NotEmptyFile(groups = Image2Checks.class)
@MaxFileSize(value = MAX_IMAGE_SIZE, unit = Unit.Kbytes, groups = Image3Checks.class)
@ImageFile(groups = Image3Checks.class)
private MultipartFile image;

// Name of this field must match with the field name that
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't it javadoc?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because this comment intended to be not for users, but for developers.

@php-coder php-coder force-pushed the download_image branch 5 times, most recently from 5025118 to 5952501 Compare May 29, 2017 21:41
@php-coder php-coder force-pushed the download_image branch 2 times, most recently from 6d56051 to 44e6e00 Compare July 21, 2017 21:49
Repository owner deleted a comment from mystamps-bot Jul 21, 2017
@php-coder php-coder force-pushed the download_image branch 2 times, most recently from 02599b6 to adeed25 Compare July 23, 2017 20:38
@php-coder php-coder force-pushed the download_image branch 2 times, most recently from 3db0f80 to fef0609 Compare July 31, 2017 13:19
@mystamps-bot
Copy link

mystamps-bot commented Jul 31, 2017 •
edited
Loading

1 Error
🚫 maven-pmd-plugin error in src/main/java/ru/mystamps/web/controller/SeriesController.java#L537-L540:
Avoid unused private methods such as ‘loadErrorsFromDownloadInterceptor(NullableImageUrl,BindingResult,HttpServletRequest)’
2 Warnings
⚠️ danger check: pull request contains 6 commits while most of the cases it should have only one. If it’s not a special case you should squash the commits into single one.
But be careful because it can destroy all your changes!
⚠️ danger check: branch download_image does not comply with our best practices. Branch name should use the following scheme: ghXXX_meaningful-name where XXX is an issue number. Next time, please, use this scheme :)
1 Message
📖 maven-pmd-plugin reported about 1 error. Please, fix it. See also: https://github.com/php-coder/mystamps/wiki/pmd-cpd

Generated by 🚫 Danger

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.4%) to 70.27% when pulling e2d734d on download_image into 22b3e5b on master.

@php-coder php-coder force-pushed the download_image branch 2 times, most recently from f31a9e8 to 6103fc8 Compare August 24, 2017 20:34
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.3%) to 69.722% when pulling 6103fc8 on download_image into 128920c on master.

@php-coder php-coder force-pushed the download_image branch 5 times, most recently from 5e4a9e6 to f53e293 Compare September 24, 2017 22:13
@php-coder
/series/add: allow to specify image URL.
7fb6289
@php-coder
/series/{id}: add label and use a standard markup for image upload fi…
376b9bb 
…eld.

Required for adding a field for specifying image URL later.

No functional changes.
@php-coder php-coder changed the title [WIP] Add ability to specify URL for an image Add ability to specify URL for an image Oct 16, 2017
@php-coder
Copy link
Owner Author

Merged in d839472 commit.

@php-coder php-coder closed this Oct 16, 2017
@php-coder php-coder deleted the download_image branch October 16, 2017 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asm0dey asm0dey asm0dey left review comments

Assignees
No one assigned
Labels
resolution/merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@php-coder @mystamps-bot @coveralls @asm0dey