msgraph-sdk-java-core icon indicating copy to clipboard operation
msgraph-sdk-java-core copied to clipboard

** fix ** fix for LargFileUploadTask constructor

Open kekolab opened this issue 1 year ago • 4 comments

LargeFileUploadTask's constructor does not check anymore whether the available() method of InputStream returns a positive number; as stated by the javadoc this method just makes an estimate of the available bytes and can return 0 even if the stream contains bytes to read.

Fixes #1621

Changes proposed in this pull request

  • In LargeFileUploadTask the check on the InputStream's bytes availability is removed.
  • Updated patch version
  • Updated CHANGELOG

kekolab avatar May 22 '24 13:05 kekolab

@baywet not a great git user, but I think I managed to obtain what you requested, although by reverting and recommitting. :$

kekolab avatar May 22 '24 13:05 kekolab

Thanks for making the update. @ramsessanchez Do you remember why this check might have been added back then? https://github.com/microsoftgraph/msgraph-sdk-java-core/pull/781

baywet avatar May 22 '24 13:05 baywet

@baywet not a great git user, but I think I managed to obtain what you requested, although by reverting and recommitting. :$

no worries, I have set the auto-merge to squash, which will result in a single commit without the undesired changes

baywet avatar May 22 '24 13:05 baywet

I am actually making a mess... I updated another file, for which I wanted to create a different PR, but it seems that everything i commit to my fork's dev branch ends up in this PR. I will stop altering my branch. I promise.

kekolab avatar May 22 '24 13:05 kekolab

Thanks for making the update. @ramsessanchez Do you remember why this check might have been added back then? #781

Documentation for available is meant to return the number of bytes that can be read before blocking, however, upon a second check it appears to always return 0 in the case of InputStream , but returns a positive value in the case of BufferedInputStream, or when bytes of the stream have been stored in memory. In our case we use InputStream so we should certainly remove this check and find a better way to ensure that InputStream is not empty. Approving the change as well.

ramsessanchez avatar May 22 '24 18:05 ramsessanchez

https://stackoverflow.com/questions/3695372/what-does-inputstream-available-do-in-java

ramsessanchez avatar May 22 '24 18:05 ramsessanchez

@baywet , removed auto merge, please give final approval.

ramsessanchez avatar May 22 '24 18:05 ramsessanchez

Just wanted to be sure you noticed you also approved and merged the change on settings.json to forbid autoformatting, in this project, a source file modified by somebody (like me) who has autoformatting enabled at global level

kekolab avatar May 22 '24 22:05 kekolab

Thanks for the callout #1627

baywet avatar May 23 '24 12:05 baywet

@baywet you're welcome. Just out of curiosity, but... why reverting it? Do not misunderstand me, please. I am not offended by the revert, but I can't help but asking: removing it, don't you risk that if somebody deletes one line, you get tons of modifications to the file because of a possible autoformatting set by the user?

I added it because I use google code style at a workspace level so when I saved LargeFileUploadTask.java, all the file was reformatted generating several "different" lines between the original and the new one (may more than the three I deleted).

Please, do not misunderstand me. I am just wondering whether there are better ways to get the modified file not corrupted by user autoformatting rules.

Just asking for learning. Thanks for the patience.

kekolab avatar May 23 '24 13:05 kekolab

For context we have continuous integration validation that checks for formatting so if any user is not set up properly we will know about that during the pull request validation. Or at least that was my assumption until you called that out. It seems this repository is not set up for linting validation just yet but here is an example PR for another repository and if you want to set it up for this repository in a full request will be more than happy to review and merge this. https://github.com/microsoft/kiota-java/pull/823

baywet avatar May 23 '24 13:05 baywet

Thanks for explaining. About integrating spotless in this project as well, why not during spare time.

kekolab avatar May 23 '24 13:05 kekolab