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

CDK: Make consts required in Pydantic generated json schemas #32251

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

flash1293
Copy link
Contributor

This PR fixes a problem @chandlerprall noticed.

For specs that are built using pydantic models, each used oneOf requires a constant property that can be used to clearly determine which option was chosen. This property needs the const setting.

In pydantic, this can be enabled via Field("val", const=True), there is a catch however: If you do this, then the field will not be required (because a default value is set, and in pydantic a field with a default value can not be required at the same time).

This works fine for the UI because it has a special handling for this case and always sets the constant property correctly, however for the terraform resource generation, this shows up as optional even though the connector won't work this way:
File

The same problem also applies to the format selection in file based sources.

This PR fixes the json schema generation for both vector db destinations and file sources.

@flash1293 flash1293 requested a review from a team as a code owner November 7, 2023 17:05
Copy link

vercel bot commented Nov 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2023 5:10pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Nov 7, 2023
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

do we also need to update connectors that might be using oneOfs in their pydantic spec?

@flash1293
Copy link
Contributor Author

@girarda after this is merged we need to roll out the change to the connectors consuming it, that's right. However we need to release the CDK before doing this.

@clnoll
Copy link
Contributor

clnoll commented Nov 8, 2023

@flash1293 the changes look good to me, but do you have a PR for the connector updates and/or can you confirm that you tested this manually locally? Do you expect there to be any UI changes (e.g. fields marked as required)?

@flash1293
Copy link
Contributor Author

@clnoll I didn't create the PR yet, but I tested locally it works as expected. There won't be UI changes as only the hidden const field is set to required (tested this with the connector form component here: https://components.airbyte.dev/?path=/story/connector-connectorform--common )

@clnoll
Copy link
Contributor

clnoll commented Nov 8, 2023

Awesome, thanks for the info. In that case LGTM!

@flash1293 flash1293 merged commit e113ff6 into master Nov 9, 2023
18 checks passed
@flash1293 flash1293 deleted the flash1293/make-consts-required branch November 9, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants