Parse Camel Case and Pascal Case Enum Values by mgyucht · Pull Request #550 · databricks/databricks-sdk-go

mgyucht · 2023-07-12T15:16:38Z

Changes

Currently, enum values that use camelCase or PascalCase are not split into separate words during the parse phase of the OpenAPI spec. As a result, PascalCase() returns Camelcase or Pascalcase, respectively. To fix this, I've modified SplitASCII to support parsing more forms (snake-case and constant-case), to split words based on all non-alphanumeric symbols, and to estimate the "case" of a number by looking at the neighboring letters.

Tests

Added several cases to SplitASCII unit tests.
Regenerated the Go, Python, and Java SDKs with this change, and the results are positive. Regenerate Python SDK using recent OpenAPI Specification databricks-sdk-py#229 and Regenerate Java SDK using recent OpenAPI Specification databricks-sdk-java#115

pietern

Nice work!

This reminds me that we still split OAuth into ["o", "auth"].

We probably need allow listing to make that work.

pietern · 2023-07-12T15:37:34Z

openapi/code/package.go

 		var splits []string
-		for _, v := range whitespace.Split(name, -1) {
+		for _, v := range SplitASCII(name) {
 			splits = append(splits, strings.Title(strings.ToLower(v)))


Per definition of SplitASCII, the chunks are already lower cased.

nfx

major comments:

splitASCII should remain a private method of Named.
there needs to be a change in Package.makeEnum

perhaps, changes to splitascii might be smaller

nfx · 2023-07-12T15:32:23Z

openapi/code/named.go

 // (?<=[a-z])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])|([-_\s])
 // and convert all words to lower case
-func (n *Named) splitASCII() (w []string) {
+func SplitASCII(name string) (w []string) {


can you revert it to being a method on Named? it's not technically needed, as what is being split gets fed into Named anyway.

btw, can you try for comparison - use Named{content, description}, with old implementation of splitASCII and see what happens

Good point on the namespacing!

If I revert to the old implementation, splitASCII fails on K8S_FAILURE and Patch:Request, producing k8, s, failure and patch:, request respectively

openapi/code/named.go

openapi/code/named_test.go

nfx · 2023-07-12T15:39:25Z

service/iam/model.go

 type PatchSchema string

-const PatchSchemaUrnIetfParamsScimApiMessagesPatchop PatchSchema = `urn:ietf:params:scim:api:messages:2.0:PatchOp`
+const PatchSchemaUrnIetfParamsScimApiMessages20PatchOp PatchSchema = `urn:ietf:params:scim:api:messages:2.0:PatchOp`


single-value enums hint at patch operations for SCIM having to be manually coded

+1. Until this is supported at the OpenAPI level, is there a practical approach we can take?

I see that there is an x-databricks-alias-enum field which we can use to simplify this name significantly, I'll do that in a separate PR since that requires an OpenAPI change in universe.

@mgyucht these enums won't be present in Java SDK - it won't allow non-alphanum values.

one of the options is an annotation for a field to always be present in the code. still - it's only for SCIM patch requests. i think it's best to hand-code all patch requests...

Yeah, we'll have to figure out how best to expose this in the API. It's a very difficult protocol to generate an SDK for... I used jackson's at-JsonProperty annotation to control serde for these fields, so I think it should be OK in Java.

but java enums don't support non-alphanum characters.

You mean the enum value names? They just need to be valid identifiers IIUC, which are alphanum + underscore and dollar sign. See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-Identifier

The "Java letters" include uppercase and lowercase ASCII Latin letters A-Z (\u0041-\u005a), and a-z (\u0061-\u007a), and, for historical reasons, the ASCII underscore (_, or \u005f) and dollar sign ($, or \u0024). The $ sign should be used only in mechanically generated source code or, rarely, to access pre-existing names on legacy systems.

I don't see any errors from IntelliJ with this change, and there are examples where underscores are allowed in enum values: https://stackoverflow.com/questions/9712977/is-it-possible-to-have-an-enum-class-with-enums-of-two-or-more-words.

codecov-commenter · 2023-07-12T16:14:23Z

Codecov Report

Patch coverage: 96.29% and project coverage change: +0.07 🎉

Comparison is base (e71ece4) 18.62% compared to head (42d89f0) 18.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
+ Coverage   18.62%   18.70%   +0.07%     
==========================================
  Files          85       85              
  Lines        9305     9315      +10     
==========================================
+ Hits         1733     1742       +9     
- Misses       7421     7422       +1     
  Partials      151      151

Impacted Files	Coverage Δ
openapi/code/package.go	`57.78% <ø> (-0.84%)`	⬇️
service/compute/model.go	`0.00% <ø> (ø)`
service/iam/model.go	`0.00% <ø> (ø)`
service/jobs/model.go	`0.00% <ø> (ø)`
service/provisioning/model.go	`0.00% <ø> (ø)`
service/sql/model.go	`0.00% <ø> (ø)`
openapi/code/named.go	`86.61% <96.15%> (+0.68%)`	⬆️
service/compute/node_type.go	`65.07% <100.00%> (ø)`

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

nfx · 2023-07-14T10:06:17Z

service/iam/model.go

 type PatchSchema string

-const PatchSchemaUrnIetfParamsScimApiMessagesPatchop PatchSchema = `urn:ietf:params:scim:api:messages:2.0:PatchOp`
+const PatchSchemaUrnIetfParamsScimApiMessages20PatchOp PatchSchema = `urn:ietf:params:scim:api:messages:2.0:PatchOp`


@mgyucht these enums won't be present in Java SDK - it won't allow non-alphanum values.

one of the options is an annotation for a field to always be present in the code. still - it's only for SCIM patch requests. i think it's best to hand-code all patch requests...

## Changes databricks/databricks-sdk-go#550 changes the way that enums are autogenerated. The last PR updating OpenAPI used a WIP version of this logic to unblock the release, but this included a small bug affecting the name of the SCIM patch schema enum.

mgyucht added 2 commits July 12, 2023 12:58

Parse camelCase or PascalCase enum values correctly

bef5fc6

Fix finally

86a0e9d

mgyucht changed the title ~~Fix enum parsing~~ Parse Camel Case and Pascal Case Enum Values Jul 12, 2023

mgyucht requested review from nfx and pietern July 12, 2023 15:17

mgyucht added 2 commits July 12, 2023 17:28

fix test

e4691a9

fix test agian

72ceab1

mgyucht requested a review from shreyas-goenka July 12, 2023 15:33

pietern approved these changes Jul 12, 2023

View reviewed changes

nfx suggested changes Jul 12, 2023

View reviewed changes

Address some changes

347bd29

mgyucht requested a review from nfx July 13, 2023 10:46

nfx approved these changes Jul 14, 2023

View reviewed changes

Merge branch 'main' into fix-enum-parsing

df7c638

nfx added the do-not-merge label Jul 17, 2023

one more test case

42d89f0

mgyucht mentioned this pull request Jul 17, 2023

Rerun OpenAPI Generator databricks/databricks-sdk-java#117

Merged

nfx removed the do-not-merge label Jul 17, 2023

nfx merged commit 7e680c5 into main Jul 17, 2023

nfx deleted the fix-enum-parsing branch July 17, 2023 13:20

nfx mentioned this pull request Jul 17, 2023

Release 0.13.0 #555

Merged

Conversation

mgyucht commented Jul 12, 2023

Changes

Tests

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 12, 2023 •

edited

Loading