Skip to content
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

Fix: Prevent NPE with autoUpload=false and multiple files are being uploaded #919

Merged
merged 6 commits into from
May 7, 2021
Merged

Fix: Prevent NPE with autoUpload=false and multiple files are being uploaded #919

merged 6 commits into from
May 7, 2021

Conversation

tulioag
Copy link
Contributor

@tulioag tulioag commented May 5, 2021

sissbruecker
sissbruecker approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

Looks good, just added some random thoughts.

...ad-flow-parent/vaadin-upload-flow/src/main/java/com/vaadin/flow/component/upload/Upload.java
.anyMatch(index -> {
final String KEY = "uploading";
JsonObject object = files.getObject(index);
return object.hasKey(KEY) && object.getBoolean(KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

The construct here is a bit unfortunate, but it seems that there is no way to ask Flow to return an optional or a default value? If we need to do this more often, we might want to introduce a helper for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a valid concern, but JsonObject is not part of Flow and it does not provide an API using Optional.

...gration-tests/src/test/java/com/vaadin/flow/component/upload/tests/NonImmediateUploadIT.java Outdated
Assert.assertEquals("No errors", element.getText());
}

private WebElement getButton(UploadElement upload, String buttonType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to see this visually to understand that it queries a button from a nested vaadin-upload-file element. Maybe the method name should reflect that it queries a button within an upload-file element, e.g. getUploadFileElementButton.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method was renamed

@tulioag tulioag force-pushed the fix/vaadin-upload/358/Upload_component_throws_NPE_upload_setAutoUpload_false_and_click_x_to_remove_file_in_the_list branch from ee04d6a to a38d927 Compare May 6, 2021 13:22
@tulioag tulioag enabled auto-merge (squash) May 7, 2021 14:58
@tulioag tulioag force-pushed the fix/vaadin-upload/358/Upload_component_throws_NPE_upload_setAutoUpload_false_and_click_x_to_remove_file_in_the_list branch from aad854a to a337434 Compare May 7, 2021 16:02
@tulioag tulioag merged commit d2ffb7d into master May 7, 2021
@tulioag tulioag deleted the fix/vaadin-upload/358/Upload_component_throws_NPE_upload_setAutoUpload_false_and_click_x_to_remove_file_in_the_list branch May 7, 2021 16:19
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 18 issues

  • MAJOR 6 major
  • MINOR 12 minor

Top 10 issues

  1. MAJOR Upload.java#L72: Either remove or fill this block of code. rule
  2. MAJOR Upload.java#L75: Either remove or fill this block of code. rule
  3. MAJOR Upload.java#L402: Remove this unused private "fireUploadInterrupted" method. rule
  4. MAJOR Upload.java#L593: Remove this unused private "getStringObject" method. rule
  5. MAJOR Upload.java#L604: Remove this unused private "getStringObject" method. rule
  6. MAJOR pom.xml#L96: Make this line start at column 9. rule
  7. MINOR Upload.java#L361: Move this method into "DefaultStreamVariable". rule
  8. MINOR Upload.java#L383: Move this method into "DefaultStreamVariable". rule
  9. MINOR Upload.java#L397: Move this method into "DefaultStreamVariable". rule
  10. MINOR Upload.java#L407: Move this method into "DefaultStreamVariable". rule

vaadin-bot pushed a commit that referenced this pull request May 7, 2021
@tulioag @vaadin-bot
Fix: Prevent NPE with autoUpload=false and multiple files are being u…
689d832 
…ploaded (#919)

Fixes: vaadin/vaadin-upload#358
@vaadin-bot
Copy link
Collaborator

Hi @tulioag , this commit cannot be picked to 19.0 by this bot, can you take a look and pick it manually?

@vaadin-bot
Copy link
Collaborator

Hi @tulioag , this commit cannot be picked to 14.7 by this bot, can you take a look and pick it manually?

@vaadin-bot
Copy link
Collaborator

Hi @tulioag , this commit cannot be picked to 14.6 by this bot, can you take a look and pick it manually?

@vaadin-bot
Copy link
Collaborator

Hi @tulioag , this commit cannot be picked to 14.5 by this bot, can you take a look and pick it manually?

tulioag added a commit that referenced this pull request May 10, 2021
@vaadin-bot @tulioag
Fix: Prevent NPE with autoUpload=false and multiple files are being u…
4005222 
…ploaded (#953)

Fixes: vaadin/vaadin-upload#358
cherry-pick: #919

Co-authored-by: Tulio Garcia <28783969+tulioag@users.noreply.github.com>
@yuriy-fix yuriy-fix mentioned this pull request Jun 10, 2021
Merged
This was referenced Jun 10, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sissbruecker sissbruecker sissbruecker approved these changes

Assignees
No one assigned
Labels
cherry-picked-20.0 need to pick manually 14.5 need to pick manually 14.6 need to pick manually 14.7 need to pick manually 19.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upload component throws NPE upload.setAutoUpload(false); and click "x" to remove file in the list
3 participants
@tulioag @vaadin-bot @sissbruecker