The changes of this generation have been run against raptor here.
https://microsoftgraph.visualstudio.com/Graph%20Developer%20Experiences/_build/results?buildId=31836&view=results

It's complaining about the todo stuff as I haven't updated the feature/v3 branch from latest metadata in a while.

I have done a second pass on this PR. Nothing stands out to me other than what we have discussed in the Teams meeting.

Sharing my comments from the meeting here too:

Just to make sure that the transfer of methods from child to base types happened as expected, please consider providing a type summary diff.

We do this as part of the .NET generation pipeline

• Script
• Pipeline

The script loads the built assembly and walks through all the types and lists methods/properties declared directly in that type. It also sorts the results for easy-to-handle diffs.

For this particular case, I think we don't want the filtering for the directly declared methods/properties, to make sure that by inheritance we get the same set after the change.

Other than that, the change looks good. I really appreciate the smaller binary size and added javadocs.

Part 1

I've tested the following calls with these changes in https://github.com/microsoftgraph/msgraph-sdk-java/pull/119/files to increase confidence in these changes:

• get me

• get users collection and access a user in the current page

• find meeting times (action), uses the updated collection pages.

• to test the use of BaseActionCollectionRequestBuilder.java
  The good news is that BaseActionCollectionRequestBuilder works well for supporting the generated changes for making the request. I couldn't verify that deserialization of the response worked as expected as I intercepted the request to correctly set the values, but this lead to a SocketTimeoutException. I did find that EnumSet is not serialized appropriately for this scenario. The following code:

// This uses an action.
public void testGetMailtips() {
		TestBase testBase = new TestBase();
		ArrayList<String> users = new ArrayList<String>();
		users.add("michael@chambele.onmicrosoft.com");
		users.add("AdeleV@chambele.onmicrosoft.com");
		EnumSet<MailTipsType> mailtips = EnumSet.of(MailTipsType.MAILBOX_FULL_STATUS, MailTipsType.MAX_MESSAGE_SIZE);
		UserGetMailTipsCollectionPage page = testBase.graphClient.me().getMailTips(users, mailtips).buildRequest().post();
		assertNotNull(page);
	}

results in the following body:

{
    "emailAddresses": ["michael@chambele.onmicrosoft.com", "AdeleV@chambele.onmicrosoft.com"],
    "mailTipsOptions": "MAILBOX_FULL_STATUS,MAX_MESSAGE_SIZE"
}

expected:

{
    "emailAddresses": ["michael@chambele.onmicrosoft.com", "AdeleV@chambele.onmicrosoft.com"],
    "mailTipsOptions": "mailboxFullStatus,maxMessageSize"
}

Opened https://github.com/microsoftgraph/MSGraph-SDK-Code-Generator/issues/321 to track this.

Part 2

Scenario: function bound to a collection that returns a collection.

<Function Name="delta" IsBound="true">
   <Parameter Name="bindingparameter" Type="Collection(graph.mailFolder)" Nullable="false" />
   <ReturnType Type="Collection(graph.mailFolder)" Nullable="false" />
</Function>

We don't generate MailFolderDeltaCollectionRequest to pass down the generic response type to BaseCollectionRequest. This means that testBase.graphClient.me().mailFolders().delta().buildRequest().get() returns T3 instead of MailFolderDeltaCollectionPage.

This may not be an issue as IDE seems to pick it up on IntelliSense

Just not on hover

Part 3 - todo

• validate that CollectionPage can be read.
• exercise entity collection reference request (BaseEntityCollectionReferenceRequest) - We generate a ApplicationCollectionReferenceRequest which doesn't appear to be used. We generate AppRoleAssignmentCollectionReferenceRequest which doesn't appear to be used (all navigation to approleassignment entities are defined with containment). These are not in scope for this PR. I don't think this is used.
• we properly emit a query parameter in request (validate add*Option)

testBase.graphClient.users().buildRequest().top(1).select("id").get(); this has the same issue as in Part 2 where the return type is T3 and not a UserCollectionPage. This will affect the user experience although and is technically a breaking change. Fortunately, existing code that expects the UserCollectionPage will not break.

Also, next page still works

		UserCollectionPage page = testBase.graphClient.users().buildRequest().top(1).select("id").get();
		UserCollectionPage nextPage = page.getNextPage().buildRequest().get();

• exercise entity collection request (BaseEntityCollectionRequest and BaseEntityCollectionRequestBuilder) - exercised this in this previous step.
• exercise reference collection page (BaseEntityCollectionWithReferencesPage).

Investigate The *CollectionWithReferencesPage looks fine. I encounter a NullPointerException when I make a request to get references:

DirectoryObjectCollectionWithReferencesPage page = testBase.graphClient.me()
									.transitiveMemberOf()
       								        .references()
									.buildRequest()
									.get();

there isn't a problem if I try a non references request:

DirectoryObjectCollectionWithReferencesPage page = testBase.graphClient.me()
									.transitiveMemberOf()
									.buildRequest()
									.get();

Investigate DirectoryObjectCollectionWithReferencesPage may be an issue when following $ref next pages as it may not instantiate the correct request object.

• exercise entity collection with references request (BaseEntityCollectionWithReferencesRequest and BaseEntityCollectionWithReferencesRequestBuilder)

Duplicate We don't emit a *CollectionWithReferencesRequest. We do create *CollectionWithReferencesRequestBuilder which is only successful without the references path. See the previous issue with the NullPointerException.

• exercise entity reference request (BaseEntityReferencesRequest and BaseEntityReferencesRequestBuilder)

I don't think we actually use BaseEntityReferencesRequest. I'm not finding anything that actually uses this template. Everything is a *WithReferencesRequest. The same with BaseEntityReferencesRequestBuilder - I don't think that template is used. It is hard to say. We really need to turn on the source template for the generated code files.

• exercise entity with reference request (BaseEntityWithReferenceRequest and BaseEntityWithReferenceRequestBuilder)

Investigate The *WithReferenceRequest does not contain a definition for GET. It only has a definition for DELETE and PUT. This may be the current state for the library.

The *WithReferenceRequestBuilder looks as expected. Even if the GET was generated as expected, I don't expect it to work right now as there appears to be an issue with reference request response payloads. I've got a question out to AGS Engineering Help channel about this.

• exercise method body request (tested with getMailTips)
• validate that deltaLinks are available on collection page results
• validate that functions work as expected (look at another example besides Collection(mailfolder).Delta() in Part 2 since they were moved out of BaseMethodCollectionRequest.
  Validated with the recent function. graphClient.me().drive().recent().buildRequest().get();

On a different note, the IntelliJ user experience when formatting request builders to minimize line length is great! It shows the return type for each line!

@baywet Does e54ebce addresses the NullPointerException?

@MIchaelMainer no it doesn't, I still have to go through the remaining items you provided with the checklist. Thanks for the amazing testing work BTW!

For clarity I'm moving the to-dos from the review here:

• Investigate The *CollectionWithReferencesPage looks fine. I encounter a NullPointerException when I make a request to get references: fixed
• Investigate DirectoryObjectCollectionWithReferencesPage may be an issue when following $ref next pages as it may not instantiate the correct request object. tested
• Investigate The *WithReferenceRequest does not contain a definition for GET. It only has a definition for DELETE and PUT. This may be the current state for the library. present https://github.com/microsoftgraph/msgraph-sdk-java/blob/temp/generic-ci-test/src/main/java/com/microsoft/graph/http/BaseWithReferenceRequest.java#L87

@MIchaelMainer I've just finished addressing the final 3 points. Let me know whether I missed anything, but this should be good for final review.