Skip to content

Conversation

@baywet
Copy link
Member

@baywet baywet commented Feb 18, 2021

This pull request cleans up some of the language in the requirement, markdown formatting and updates the java samples.

Items left to discuss:

  • Should we align the object model naming in the spec? (currently ChunkedUploadProvider is not really informative for Java)
  • Should the task try to get the result (issuing a sub-query) after the upload completes on an empty response with a location header pointing to a resource on graph?
  • Alternatively, should the task return a wrapper object instead of the direct type, so we could provide the type whenever available, or response information when not available?

We can have the discussion here before we merge that pull request. This way I can make subsequent updates before we merge this PR.

@baywet baywet self-assigned this Feb 18, 2021
@baywet
Copy link
Member Author

baywet commented Feb 18, 2021

related work microsoftgraph/msgraph-sdk-javascript#392

Copy link
Contributor

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

With regards to the first query. I agree we should try to align the object modelling where we can.

@baywet
Copy link
Member Author

baywet commented Feb 19, 2021

@andrueastman @nikithauc What about aligning on LargeFileUploadXXX (Provider, result, etc...) ?
Another subsequent question I forgot to ask. What about guiding the implementers in terms of namespace naming. Currently in Java it lives in "concurrency" and without looking in the Microsoft Graph Docs or knowing this before hand, I'd never think it's in there.

@andrueastman
Copy link
Contributor

With regards to namespace naming, I think it best lives in the same place as the PageIterator which is also a Task.

@baywet
Copy link
Member Author

baywet commented Feb 22, 2021

which we don't have in Java. So the guidance would be a tasks sub namespace under the root one then?

@baywet
Copy link
Member Author

baywet commented Feb 22, 2021

I just pushed another commit making the changes we've been talking about. Let me know what you think.


// create an upload session
const uploadTask = await MicrosoftGraph.OneDriveLargeFileUploadTask.create(client, file, options);
const uploadTask = await MicrosoftGraph.LargeFileUploadTask.create(client, file, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no create() function in LargeFileUpload task. It is only in OneDriveFileUpload. Please undo this for the time being. I will update this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal of this document is to describe the future state of the solution, not the current one. I'd like this spec to be as final as possible so we each have clear guidelines on what changes need to be implemented in each SDK. The Java samples also do not map to current state here, this is expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

LargeFileUploadTask.createUploadSession(client: Client, requestUrl, payload, headers) this is what is present.

Copy link
Member Author

Choose a reason for hiding this comment

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

which is the equivalent of graphClient.me().drive().item("blah.blob").createUploadSession() and is present as a static method because the JS SDK doesn't have a fluent API yet, correct?

What's the method to create an upload task from the session? a ctor like new LargeFileUploadTask(session, stream, options) ? And then task.upload()?

@nikithauc
Copy link
Contributor

I came across this issue in JavaScript microsoftgraph/msgraph-sdk-serviceissues#39.
The non uniform casing had to be handled in JavaScript as JSON property name is case sensitive. I don't think C# has this issue though.

@andrueastman
Copy link
Contributor

Yeah. The deserializer in C# is set to be case insensitive on properties.

Copy link
Contributor

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

This looks good to me now once we incorporate the comments from @nikithauc

@baywet
Copy link
Member Author

baywet commented Feb 23, 2021

@nikithauc Oh yeah! I remember running into the same issue when working on the Java one. I'm going to add a warning in the document to tell implementers to plan for that. Thanks for the reminder.

Co-authored-by: Nikitha Chettiar <[email protected]>
@baywet baywet requested a review from nikithauc February 23, 2021 19:21
@baywet
Copy link
Member Author

baywet commented Feb 24, 2021

Thanks for the discussion! Merging. @nikithauc I haven't created issues in the JS repo to align on the spec because I know you already have multiple issues tracking work on that front. Let me know if I should create one like I did for java and dotnet.

@baywet baywet merged commit 33ce73a into master Feb 24, 2021
@baywet baywet deleted the feature/upload-task-java-cleanup branch February 24, 2021 13:15
@nikithauc
Copy link
Contributor

Thanks for the discussion! Merging. @nikithauc I haven't created issues in the JS repo to align on the spec because I know you already have multiple issues tracking work on that front. Let me know if I should create one like I did for java and dotnet.

Thank you @baywet! The discussion on upload handling came up because of bugs reported in the JS repo. So we already have issues open for upload updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants