Refactor Handling of Name<->ID Mapping in OpenAPI Generator #547

mgyucht · 2023-07-10T15:19:40Z

Changes

This PR proposes a refactor to the x-databricks-id and x-databricks-name extensions to the OpenAPI specification to support schema reuse while still providing the name <-> ID mapping features.

Many resources in Databricks have user-facing names (like job name, cluster name, etc.). However, for many API calls, this name cannot be used to identify the resource; instead, a system-generated resource ID must be used instead (like job ID, cluster ID, etc.). For resources that support listing, the presence of these two extensions in the response body triggers the generation of additional code in the SDKs as convenience features for interacting with these resources, namely:

GetByName methods that allow looking up a resource by its user-facing name, and
XToYMap methods that materialize a mapping from name to resource.

Previously, these extensions were defined at the level of a schema, where a field within a schema marked with these extensions would be a candidate for the above functionality. However, there is a problem with this when a component is reused in multiple APIs. First, it is possible to define a single field as a component. This is useful if the field is present in different related resources but the documentation/type of the field should be consistent. Beyond this, it is possible for a list response to contain multiple fields corresponding to IDs to Databricks resources, but in this scenario, only one of those IDs is actually the ID of the underlying resource. If the response body contains two or more fields marked with x-databricks-id or x-databricks-name, it is impossible for our code generator to know which was intended as the ID/Name for the resource. Furthermore, there are instances where these extensions are not used correctly: users would annotate path parameters with these extensions, but these only make sense in the context of a list response.

Ultimately, the fields used for name and ID are specific to the operation itself and not to the schema. This allows multiple ID/name fields to be reused as components within a listing response, and the definition of which to use for name-to-ID mapping is fixed at the operation level. This way, different operations can define different fields to use for this mapping.

This PR implements this change by moving the name & ID fields from the Schema level to the Operation level. The type of this extension changes from a boolean to a list of strings, corresponding to the path of fields to be traversed to get to the respective field in the response entity. The starting point for this traversal is the underlying resource itself, as opposed to the response body. Additionally, it adds support for nested ID fields (by preserving the implementation between the treatment of name and ID). Unfortunately, it also updates the Go SDK to a relatively recently OpenAPI commit as well, so there are additional changes here.

Next steps:

Update OpenAPI spec in Universe, refactoring these extensions to the Operation level.
Update Go SDK with this change.
Update Python SDK with this change.
Update Java SDK with this change.
Update go/openapi/spec with this change.

Tests

make test passing
make fmt applied
relevant integration tests applied

codecov-commenter · 2023-07-11T15:56:35Z

Codecov Report

Patch coverage: 5.12% and project coverage change: -0.10 ⚠️

Comparison is base (c37a894) 18.72% compared to head (2c65f12) 18.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
- Coverage   18.72%   18.62%   -0.10%     
==========================================
  Files          85       86       +1     
  Lines        9233     9304      +71     
==========================================
+ Hits         1729     1733       +4     
- Misses       7355     7420      +65     
- Partials      149      151       +2

Impacted Files	Coverage Δ
openapi/code/entity.go	`29.72% <0.00%> (ø)`
openapi/code/method.go	`18.03% <0.00%> (+2.31%)`	⬆️
openapi/code/package.go	`58.62% <ø> (-0.41%)`	⬇️
openapi/extensions.go	`0.00% <0.00%> (ø)`
openapi/model.go	`5.97% <0.00%> (-0.28%)`	⬇️
service/billing/model.go	`0.00% <ø> (ø)`
service/catalog/api.go	`0.00% <0.00%> (ø)`
service/catalog/impl.go	`0.00% <0.00%> (ø)`
service/catalog/model.go	`0.00% <ø> (ø)`
service/compute/api.go	`13.28% <ø> (ø)`
... and 10 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mgyucht · 2023-07-11T12:17:14Z

openapi/code/method.go

+type fieldPath []*Field
+
+func (fp fieldPath) Name() *Named {
+	var parts []string
+	for _, f := range fp {
+		parts = append(parts, f.CamelName())
+	}
+	return &Named{Name: strings.Join(parts, "")}
+}


Can this be removed?

mgyucht · 2023-07-11T12:18:25Z

openapi/code/method.go

+func (m *Method) IdentifierField() *Field {
+	if !m.HasIdentifierField() {
+		return nil
+	}
+	return m.IdFieldPath[len(m.IdFieldPath)-1]
+}
+
+func (m *Method) IdentifierName() string {
+	if !m.HasIdentifierField() {
+		return ""
+	}
+	return m.IdFieldPath[len(m.IdFieldPath)-1].PascalName()
+}


Are these used in templates?

Maybe get rid of second

mgyucht · 2023-07-11T12:19:07Z

openapi/code/method.go

-	return p.Offset != nil && p.Entity.HasIdentifierField()
+func (m *Method) NeedsOffsetDedupe() bool {
+	p := m.Pagination()
+	return p.Offset != nil && len(m.IdFieldPath) != 0


use above func

mgyucht · 2023-07-11T12:23:26Z

openapi/code/service.go

+
+	var nameFieldPath, idFieldPath []*Field
+	if op.HasNameField() {
+		respEntity := response.Field(op.Pagination.Results).Entity


Handle case where either pagination or pagination.results are not set

pietern

LGTM. Please fix chmod of service/billing/model.go before merge.

First cut, seems to work

e064b06

mgyucht changed the title ~~Refactor Handling of Name<->ID Mapping in OpenAPI Generator~~ [WIP] Refactor Handling of Name<->ID Mapping in OpenAPI Generator Jul 10, 2023

mgyucht requested review from nfx and pietern July 10, 2023 15:20

mgyucht added 11 commits July 10, 2023 21:38

more regen

09e15f0

more

f02cbeb

Migrate workspaces

d1b1dfd

more

c1e685c

next batch

d2c94e8

more

840e93c

jobs

65554b0

metastore

3318cf0

more migrations

e4636a0

finish migration

074cd32

last bug fixes

0640468

mgyucht changed the title ~~[WIP] Refactor Handling of Name<->ID Mapping in OpenAPI Generator~~ Refactor Handling of Name<->ID Mapping in OpenAPI Generator Jul 10, 2023

mgyucht added 5 commits July 10, 2023 23:28

Merge branch 'main' into es-770372

462a49c

fix

ed0f478

more changes

2de2d3c

minimize diffs

0be9f62

more fixes

2c65f12

remove unnecessary diffs

61feb99

mgyucht commented Jul 11, 2023

View reviewed changes

pietern approved these changes Jul 12, 2023

View reviewed changes

address commentg

2f6f6e3

mgyucht merged commit f84de61 into main Jul 12, 2023

mgyucht deleted the es-770372 branch July 12, 2023 07:58

nfx mentioned this pull request Jul 17, 2023

Release 0.13.0 #555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Handling of Name<->ID Mapping in OpenAPI Generator #547

Refactor Handling of Name<->ID Mapping in OpenAPI Generator #547

mgyucht commented Jul 10, 2023 •

edited

Loading

codecov-commenter commented Jul 11, 2023 •

edited

Loading

mgyucht Jul 11, 2023

mgyucht Jul 11, 2023

mgyucht Jul 11, 2023

mgyucht Jul 11, 2023

mgyucht Jul 11, 2023

pietern left a comment

Refactor Handling of Name<->ID Mapping in OpenAPI Generator #547

Refactor Handling of Name<->ID Mapping in OpenAPI Generator #547

Conversation

mgyucht commented Jul 10, 2023 • edited Loading

Changes

Tests

codecov-commenter commented Jul 11, 2023 • edited Loading

Codecov Report

mgyucht Jul 11, 2023

Choose a reason for hiding this comment

mgyucht Jul 11, 2023

Choose a reason for hiding this comment

mgyucht Jul 11, 2023

Choose a reason for hiding this comment

mgyucht Jul 11, 2023

Choose a reason for hiding this comment

mgyucht Jul 11, 2023

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

mgyucht commented Jul 10, 2023 •

edited

Loading

codecov-commenter commented Jul 11, 2023 •

edited

Loading