Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data API V2: Make adjustments to align SDK data between v1 and v2 #3353

Merged
merged 10 commits into from
Nov 17, 2023

Conversation

stephybun
Copy link
Member

No description provided.

@stephybun stephybun changed the title Data API V2: Make adjustments to align data between v1 and v2 Data API V2: Make adjustments to align SDK data between v1 and v2 Nov 15, 2023
@stephybun stephybun requested a review from a team November 15, 2023 13:45
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

As discussed offline, there's a couple of things we should look to fix out-of-band, but otherwise this LGTM 👍

tools/data-api/internal/endpoints/v1/mappings.go Outdated Show resolved Hide resolved
tools/sdk/resourcemanager/service_version.go Show resolved Hide resolved
Comment on lines 43 to 50
// disregard the response object if it's either a long running operation or not a list operation
if !input.LongRunning || input.FieldContainingPaginationDetails != nil {
responseObject, err := mapObjectDefinition(input.ResponseObject, knownConstants, knownModels)
if err != nil {
return nil, fmt.Errorf("mapping the response object definition: %+v", err)
}
output.ResponseObject = responseObject
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In V1, we're outputting the Response Object into the API Definitions and then having the Data API handling transforming this - the intention was that we could decide later on if we wanted to pipe this through or not.

As you're doing here, V1 currently removes the Response Object from a Long Running Operation when it's mapped - however since supporting returning the Response Object from a Long Running Operation requires a larger set of changes, rather than stripping this out in the Data API, the importer should do this instead - as such I'll send a separate PR for this shortly.

However - the Response Object is necessary for a List Operation - in the V1 API Definitions the base type used for a ListOperation expects that each operation outputs this into the NestedItemType field which is returned from the ResponseObject method. This is for historical reasons, where the ListOperations were treated differently - but ultimately the NestedItemType field gets pulled out into the ResponseObject field - meaning that we need to include this.

As such can we remove this logic for the moment? Whilst that's going to be temporarily incorrect, can we update both the Importer and Data API V1 so that we don't output these values (update: #3364 is opened) - which'll fix this issue going forwards.

Suggested change
// disregard the response object if it's either a long running operation or not a list operation
if !input.LongRunning || input.FieldContainingPaginationDetails != nil {
responseObject, err := mapObjectDefinition(input.ResponseObject, knownConstants, knownModels)
if err != nil {
return nil, fmt.Errorf("mapping the response object definition: %+v", err)
}
output.ResponseObject = responseObject
}

@stephybun stephybun merged commit 66764c2 into main Nov 17, 2023
1 check passed
@stephybun stephybun deleted the f/further-data-api-fixes branch November 17, 2023 12:50
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.

2 participants