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

Consider default values for oneOf fields in connector specs #17352

Closed
lmossman opened this issue Sep 28, 2022 · 8 comments · Fixed by #20588
Closed

Consider default values for oneOf fields in connector specs #17352

lmossman opened this issue Sep 28, 2022 · 8 comments · Fixed by #20588
Assignees
Labels
area/connectors Connector related issues area/frontend Related to the Airbyte webapp from/connector-ops team/extensibility

Comments

@lmossman
Copy link
Contributor

lmossman commented Sep 28, 2022

What

See original slack thread for context.

Currently our frontend code does not consider the default field if it is set on a oneOf property in the connector specification. We should update our code to use these default values if they are set when a user first creates a connector, and set the dropdowns to those default values automatically in that case.

How

This would likely require changes in several places. Here is a draft PR with some progress started on these changes, though this still likely requires more testing and iteration: https://github.com/airbytehq/airbyte/pull/17351/files

One open question: In the case of oneOfs of objects, does the default need to contain default values for every sub-field as well? Or is there some way in the json schema that we can just say default: X where X is one choice in the oneOf, without needing to also provide defaults for all of its children?

@lmossman lmossman added area/frontend Related to the Airbyte webapp from/connector-ops labels Sep 28, 2022
@octavia-squidington-iv
Copy link

cc @airbytehq/frontend

@rodireich rodireich added the area/connectors Connector related issues label Sep 28, 2022
@swyxio
Copy link
Contributor

swyxio commented Sep 28, 2022

more broadly i wonder if we can use a jsonschema interpreter library that will at least ensure we are "up to spec" whatever that means

@timroes
Copy link
Collaborator

timroes commented Sep 28, 2022

We are using types for jsonschema in Typescript via a library (there's not much to parse since JSON schema is already plain JSON itself). This though won't solve the problem that we need to generate our UI and forms from that spec, which a library won't help us with.

@flash1293
Copy link
Contributor

In the context of #14250 I would like to close this one because a connector developer can always achieve the same by reordering their oneOfs to put the first one on top.

This is a preferable solution because it makes the UI more consistent - if there is a oneOf, the default selection will always be the top one. Preselecting another one that's not the default could be slightly unexpected/confusing for a user.

Instead I would like to enforce no default values on oneOfproperties via SAT (already added it to this issue: #20063)

What do you think @lmossman ?

@flash1293
Copy link
Contributor

Discussed with @rodireich and we concluded that there are good use cases for this functionality. I checked through our existing specs and it turns out a bunch of them are already implementing default values. All of them specify the value of the shared const field, so I'm going to implement this behavior.

@rodireich
Copy link
Contributor

Thanks @flash1293
This was in the context of #16664 from a few months back.
Once a default is implemented I can go back and improve they way our config UI behaves.

@flash1293
Copy link
Contributor

Approved, waiting for the code freeze

@flash1293
Copy link
Contributor

@rodireich This should work now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/frontend Related to the Airbyte webapp from/connector-ops team/extensibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants