Skip to content

Do not use cached entities in code generation#403

Merged
andrewnester merged 1 commit intomainfrom
no-entity-cache
May 25, 2023
Merged

Do not use cached entities in code generation#403
andrewnester merged 1 commit intomainfrom
no-entity-cache

Conversation

@andrewnester
Copy link
Copy Markdown
Contributor

Changes

Code generator stores the defined types (entities) in pkg.types field. This field is primarily used for keeping the list of all observed types while generating code.

But if we use the entity stored in this map and modify it in-place, for example, add that some field is required, it will change the entity stored.

If later one we use this "cached" version of entity, it will contain modified configuration which can lead to unexpected results, for example, User entity in Users.create operation will have ID field as required even though based on OpenAPI definition it is not.

Thus, it's better not to use cached entities.

Tests

  • make test passing
  • make fmt applied

@andrewnester andrewnester requested a review from pietern May 25, 2023 10:31
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04 ⚠️

Comparison is base (d722c3c) 19.15% compared to head (ac19341) 19.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #403      +/-   ##
==========================================
- Coverage   19.15%   19.11%   -0.04%     
==========================================
  Files          82       82              
  Lines        8683     8679       -4     
==========================================
- Hits         1663     1659       -4     
  Misses       6874     6874              
  Partials      146      146              
Impacted Files Coverage Δ
openapi/code/package.go 58.12% <100.00%> (-0.81%) ⬇️

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

Copy link
Copy Markdown
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

For my info, where are these entities modified after being returned?

@andrewnester
Copy link
Copy Markdown
Contributor Author

@pietern as an example, for Users.* operation we were returning cached object here https://github.com/databricks/databricks-sdk-go/blob/main/openapi/code/service.go#L139

It happens that Users.update and such are processed before Users.create operation in code generator.

When we process Users.update, request object was modified here
https://github.com/databricks/databricks-sdk-go/blob/main/openapi/code/service.go#L172

Thus, when Users.create is processed and takes request schema from cache, it contains wrong required fields.

@andrewnester andrewnester merged commit 0af0ffc into main May 25, 2023
@andrewnester andrewnester deleted the no-entity-cache branch May 25, 2023 11:31
nfx added a commit that referenced this pull request Jun 1, 2023
@nfx
Copy link
Copy Markdown
Contributor

nfx commented Jun 1, 2023

this PR breaks SDK generation with path parameters

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.

4 participants