ByApplicationId is misleading: does not take an application ID

[mbarnes]

A recent code generation commit changed the naming of functions that look up an object by object ID.

Among those changes was the applications interface.

Whereas in past versions there was

func (m *GraphBaseServiceClient) ApplicationsById(id string) ...

now the equivalent function is

func (m *ApplicationsRequestBuilder) ByApplicationId(applicationId string) ...

Problem is this new function is very misleading. Application IDs are their own thing in Active Directory, separate from Object IDs. I'm pretty sure the function still wants the Object ID for an application object, since that's what the REST API wants, but both the function name and parameter name suggest the opposite.

Clarification is needed here.

[baywet]

Thanks for using the Go SDK and for reaching out.

This is an interesting side effect/edge case of naming conventions. We moved the indexer methods from the parent request builder to the item request builder to follow the path segmentation of the API:

• Before /users/id was .UsersById missing one path segment.
• Now /users/id is .Users.ByUserId mapping two code segments to two API path segments.

The "user Id" part of the second segment comes from OpenAPI.Net.OData, the conversion library we use to get an OpenAPI description of the service. The reason why the parameter is named "Entity-id" and not simply "Id" is to avoid collisions in the URL template (/groups/{group-id}/members/{member-id} instead of /groups/{id}/members/{id} which would contain twice the same parameter name for two different parameters).

Now, in the case of applications, Application Id means something very different from the object Id, and I agree that naming the object ID "application Id" will lead to confusions. Unfortunately this is something we're not likely to change anything soon since it'd represent a binary breaking change and Go is now generally available.

Our only remaining option would probably be the documentation and make sure this comment reflects we're referring to the object ID and not the application ID. However I'm not sure we have a way to annotate this in the metadata today. @irvinesunday Do you have any idea? (description/comment for the entity set index parameter)

https://github.com/microsoftgraph/msgraph-sdk-go/blob/37c5cc8f296d36c2e66ab0b02ed25097d161dc44/applications/applications_request_builder.go#L49

[mbarnes]

If the ID parameters in the item request builders are always referring to object IDs, renaming the parameters from (in this case) applicationId to applicationObjectId would be sufficiently clarifying.

[baywet]

right, but doing so would be a binary breaking change which we'd like to avoid.

[mbarnes]

Just the parameter names would be a breaking change? I wasn't suggesting renaming the functions.

[baywet]

Both the parameter name and the function name are generated and driven by the same input, which is ultimately the entity name + a naming convention.

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[tonyjthomas]

To add anecdotal data - this seems completely worth making a breaking change for. It's not merely confusing or inconvenient, it's straight-up wrong. It's perfectly normal in various contexts to query by the actual application id (client id), e.g. az ad app list --app-id <application-id>, and this method strongly suggests that that is what it wants.

I was on the verge of concluding that this SDK was busted and scrapping it in favor of shelling out to the Az CLI when I found this thread. I'm relieved that querying by Object ID works, but never in a million years would I have thought to try that for a method explicitly named ByApplicationId.

ById, ByObjectId, or even ApplicationById are all names that would get me to think twice and try both fields - ByApplicationId leaves no ambiguity.

[baywet]

For anyone who comes back to that issue, both are supported today

1. getting an application using the object id client.Applications.ByApplicationId("appId").Get(context.Background())
2. getting an application using the app id client.ApplicationsWithAppId("appId").Get(context.Background())