Content Length Missing Fix

[ramsessanchez]

There have been several issues raised concering the failure of FileUpload. The issue stems from the fact that we did not include an Override on the contentLength method on the RequestBody custom constructor when translating a RequestInformation object to an Okhttp Request object. I have also added a property on RequestInformation that is set whenever the content property is set on the RequestInformation object. While we did include a content-length header it seems to also be a requirement that this property is set on the RequestBody otherwise we will receive an error stating that the 'content length is unknown'.

This will lead to version 1.0.3

[andreaTP]

I'm not concerned about breaking the API in this case, as almost anything we are going to do on the InputStream is gonna throw an IOException.

I believe that relying on available is not the correct resolution for this bug.

Having a test that stresses the use case would be great.

[ramsessanchez]

I have refactored the code to utilize Okhttp's method RequestBody.create(Byte[], MediaType)
Using the functionality made available in Compatability.readAllBytes[] we can convert the inputStream to a byte array which we then pass to the create function, this removes the need to Overwrite any of the methods and rather use OkHttp's native implementation. Further, by converting the input stream to a byte[] array we should be able to more reliably get the length of the stream without using the available method which @andreaTP pointed out can have unwanted affects if the inputStream does not correctly implement the method. I have wrapped the ioException in a runtime exception, though that is temporary as I would like to hear everyone's thoughts as to how best we should handle this exception.

[araneolus]

With this solution, is it no longer necessary to know the length of the InputStream in advance? The background is that in my program I first write the InputStream into a ByteArray to determine the final length, which I do not know, because I get the data from different data sources.
So far I have to set the length in the "DriveItemUploadableProperties".
If now the OkHttp method does the same, I have a doubling of the data in the memory which is not exactly resource-saving.

[calebkiage]

@araneolus, I would advise caution if you don't control the source giving you the unknown length streams. The reason is that a misbehaving server can send you an infinite stream, which would fill up your RAM and cause a DoS. A potential change is to read from the stream into a fixed length buffer, then write the buffer contents into a file (up to a limit). You can get the size of the stream by either calculating it or from the file's metadata. You could then upload the file after.

[baywet]

Get close! I'd like unit tests to be implemented for both scenarios:

• the stream is coming from local file system
• the stream is a known JSON payload

To make sure the headers are what we expect

[github-actions]

Conflicts have been resolved. A maintainer will take a look shortly.

[baywet]

Thanks for making the changes!

[baywet]

@ramsessanchez there are still a few failing unit test if you could look into it please?

[sonarcloud]

Quality Gate failed

Failed conditions
58.6% Coverage on New Code (required ≥ 80%)
5.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

[araneolus]

Hooray, it works :-)

[araneolus]

Unfortunately I have to take back my Hooray :-(
The following error occurs with large files.

Can't write 'testfolder/jdk1.6.0_45.tgz' Range [5242880, 5242880 + 5242880) out of bounds for length 5242880

I am setting the size of the File at driveItemUploadableProperties

[andreaTP]

I'm so sorry I have not followed up on this subject, but, as noticed here: #1088 (comment)

available is NOT the API you need to use in this context since it has very very different semantics from the expected behavior.
Have you explored different solutions?
Do we have a reproducer of the original issue that triggered those changes?

[andreaTP]

Unfortunately, due to the support and semantics, I'm using workarounds in the Apicurio Registry testsuite to properly perform file uploads.
But I'm looking forward to a real resolution of the issue.

If you need me to chip in I would love a minimal repoducer, maybe @araneolus would be able to provide one 🙏

[araneolus]

Ok, This is my Test code.
I have two issues with this code.

• If I use the tempFilePath and the Data is smaller 4M, the method "sendPrimitive" is running on a Exception under windows "the process cannot access the file because it is being used by another process" It works well with the ByteArray tempBuffer.
• If the file is larger than 4M I got the error "Can't write 'testfolder/jdk1.6.0_45.tgz' Range [5242880, 5242880 + 5242880) out of bounds for length 5242880"

public void put(final OFSFile<?> ofsFile, final String destination, final InputStream fis) throws IOException {
	try {
		connect();
	} catch (InterruptedException e) {
		throw new IOException("Interrupted");
	}
	Drive drive = this.getDrive();
	if (drive == null) {
		throw new IOException("Drive is empty");
	}
	Path tempFilePath = null;
	long streamSize = 0;
	byte[] tempBuffer = null;
	if (this.isTempFileUse()) {
		tempFilePath = Files.createTempFile("vec", ".tmp");
		try {
			Files.copy(fis, tempFilePath);
			streamSize = Files.size(tempFilePath);
		} catch (Exception e) {
			log.error("[put] " + this.getSessionId() + " Can't create tempfile '" + tempFilePath.getFileName()
					+ "' " + e.getMessage());
			throw new IOException("Can't create tempfile '" + tempFilePath.getFileName() + "' " + e.getMessage());
		} finally {
			fis.close();
		}
		streamSize = Files.size(tempFilePath);
	} else {
		tempBuffer = IOUtils.toByteArray(fis);
		streamSize = tempBuffer.length;
	}
	InputStream inputstream = null;
	try {
		if (this.isTempFileUse()) {
			inputstream = Files.newInputStream(tempFilePath, java.nio.file.StandardOpenOption.READ);
		} else {
			inputstream = new ByteArrayInputStream(tempBuffer);
		}
		IProgressCallback progressCallback = new IProgressCallback() {
			@Override
			public void report(final long current, final long max) {
				log.debug(String.format("Uploaded %d bytes of %d total bytes", current, max));
			}
		};
		if (streamSize < 4194304) {
			RequestInformation requestInformation = graphClient.drives().byDriveId(drive.getId()).items()
					.byDriveItemId("root:/" + destination + ":").content().toPutRequestInformation(inputstream);
			URI uriIncludesConflictBehavior = new URI(
					requestInformation.getUri().toString() + "?@microsoft.graph.conflictBehavior=replace");
			requestInformation.setUri(uriIncludesConflictBehavior);
			graphClient.getRequestAdapter().sendPrimitive(requestInformation, null, InputStream.class);
		} else {

			DriveItemUploadableProperties driveItemUploadableProperties = new DriveItemUploadableProperties();
			driveItemUploadableProperties.setName(FilenameUtils.getName(destination));
			driveItemUploadableProperties.setFileSize(streamSize);

			Map<String, Object> additionalData = new HashMap<>();
			additionalData.put("@microsoft.graph.conflictBehavior", "replace");
			driveItemUploadableProperties.setAdditionalData(additionalData);

			Map<String, Object> commitData = new HashMap<>();
			commitData.put("deferCommit", false);
			
			CreateUploadSessionPostRequestBody uploadSessionPostRequestBody = new CreateUploadSessionPostRequestBody();
			//uploadSessionPostRequestBody.setAdditionalData(commitData);
			uploadSessionPostRequestBody.setItem(driveItemUploadableProperties);
			
			UploadSession uploadSession = null;
			try {
				RequestInformation test = graphClient.drives().byDriveId(drive.getId()).items()
						.byDriveItemId("root:/" + destination + ":").createUploadSession()
						.toPostRequestInformation(uploadSessionPostRequestBody);
				String xx = IOUtils.toString(test.content, StandardCharsets.UTF_8);
				
				uploadSession = graphClient.drives().byDriveId(drive.getId()).items()
						.byDriveItemId("root:/" + destination + ":").createUploadSession()
						.post(uploadSessionPostRequestBody);
			} catch (ApiException gse) {
				if (gse.getResponseStatusCode() == 404) {

				}
			}

			LargeFileUploadTask<DriveItemUploadableProperties> uploadTask = new LargeFileUploadTask<>(
					graphClient.getRequestAdapter(), uploadSession, inputstream, streamSize,
					DriveItemUploadableProperties::createFromDiscriminatorValue);

			try {
				UploadResult<DriveItemUploadableProperties> uploadResult = uploadTask
						.upload(this.getRetry() <= 0 ? 5 : this.getRetry(),progressCallback);
				if (uploadResult.isUploadSuccessful()) {
					log.info("[put] " + this.getSessionId() + "finished. File '" + destination
							+ "' succcessfully written");
				}
			} catch (ApiException | InterruptedException exception) {
				log.error("[put] " + this.getSessionId() + "Exception :" + exception.getMessage());
				exception.printStackTrace();
			}
		}
	} catch (Exception e) {
		log.error("[put] " + this.getSessionId() + " Can't write '" + destination + "' " + e.getMessage());
		throw new IOException("Can't write '" + destination + "' " + e.getMessage());
	} finally {
		if (tempFilePath != null) {
			Files.delete(tempFilePath);
		}
		if (inputstream != null) {
			inputstream.close();
		}
		tempBuffer = null;
	}
	return;
}