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

Handle enum-like oneOfs #3385

Closed
cgardens opened this issue May 12, 2021 · 4 comments · Fixed by #4917
Closed

Handle enum-like oneOfs #3385

cgardens opened this issue May 12, 2021 · 4 comments · Fixed by #4917
Assignees
Labels
priority/high High priority type/enhancement New feature or request

Comments

@cgardens
Copy link
Contributor

cgardens commented May 12, 2021

Tell us about the problem you're trying to solve

For some connector configurations occasionally we have cases where based on a configuration choice a user may (or may not need to enter more information.

e.g. if a user if choosing between file_source type in the file source, they might choose between public web, local fs, and gcp. In the public web and local fs case, simply selecting public web or local fs is enough. No extra info is required in the gcp case the user must also provide a service account.

We use a bit of a hack right now to model this:

"provider": {
        "type": "object",
        "description": "Storage Provider or Location of the file(s) to be replicated.",
        "default": "Public Web",
        "oneOf": [
          {
            "title": "HTTPS: Public Web",
            "required": ["storage"],
            "properties": {
              "storage": {
                "type": "string",
                "enum": ["HTTPS"],
                "default": "HTTPS"
              }
            }
          },
          {
            "title": "GCS: Google Cloud Storage",
            "required": ["storage"],
            "properties": {
              "storage": {
                "type": "string",
                "enum": ["GCS"],
                "default": "GCS"
              },
              "service_account_json": {
                "type": "string",
              }
            }
          },
          {
            "title": "Local Filesystem (limited)",
            "required": ["storage"],
            "properties": {
              "storage": {
                "type": "string",
                "enum": ["local"],
                "default": "local"
              }
            }
          }
        ]
      }

The issue is that with oneOf we can't have multiple "types" that match the same pattern. So public web and local fs can't just be empty objects. The way we handle this is by adding a storage field to each object in the oneOf and then having the field be an enum with one value. This passes validation because the enums do not overlap, so different objects cannot get mistaken as each other. When interacting with the object that matches this schema, we are okay because the storage field is essentially an enum.

The downside of this hack is that in the UI a user is presented with a dropdown for the storage field that has only one item in it. It implies there's a choice being made here, but there is none, so it is just confusing.

Screen Shot 2021-05-12 at 2 55 24 PM

Describe the solution you’d like

  • I want to maintain the enum-like behavior we have.
  • I want the user to not have an option to set this value.

Describe the alternative you’ve considered or used

Possible Solution 1

We could have the UI recognize when the enum for a field has only one value. In those cases it could make sure that the dropdown for that value is not editable.

Possible Solution 2

We could stop using the enum hack for this and instead use the jsonschema constants (keyword const). The UI would know not to display const fields and would by default set the value for that field to the value of the const.

Possible Solution ?

┆Issue is synchronized with this Asana task by Unito

@cgardens cgardens added the type/enhancement New feature or request label May 12, 2021
@cgardens cgardens changed the title Handle oneOfs Handle enum-like oneOfs May 12, 2021
@cgardens cgardens added this to the Core - 2021-06-23 milestone Jun 14, 2021
@cgardens cgardens removed this from the Core - 2021-06-23 milestone Jun 23, 2021
@sherifnada
Copy link
Contributor

@cgardens const seems like the better choice here. The UI could choose to display it or not as long as the config sent from the UI to the API contains that const value. It seems like the right sequencing here is:

  1. UI PR is merged to handle const values
  2. Connector specs are migrated (should be completely backwards compatible)

Related #3961

@cgardens
Copy link
Contributor Author

Agreed. @jamakase would it be possible for us to move forward with "Possible Solution 2" in such a way that is backwards compatible with what we have now? basically I agree with Sherif's proposal on sequencing. Want to make sure the UI work is done in such a way that it is possible.

@cgardens cgardens added the priority/low Low priority label Jun 25, 2021
@cgardens cgardens added this to the Frontend - 2021-06-28 milestone Jun 25, 2021
@cgardens
Copy link
Contributor Author

@sherifnada do you think there is any sort of static checking we could do at build time? i agree with the convention you want to go for. wondering how we will make this obvious to new engineers and contributors. maybe we just document it? 🤷‍♀️

@sherifnada
Copy link
Contributor

we can:

  1. document it
  2. put a $comment in generated spec.jsons pointing to the docs for how to write a spec
  3. put a SAT test case verifying that oneOfs don't use the pattern above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high High priority type/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants