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

SAT: Add tests for all JSON schema features that are not supported by UI #20063

Closed
flash1293 opened this issue Dec 5, 2022 · 5 comments · Fixed by #21451
Closed

SAT: Add tests for all JSON schema features that are not supported by UI #20063

flash1293 opened this issue Dec 5, 2022 · 5 comments · Fixed by #21451
Assignees
Labels
area/frontend Related to the Airbyte webapp from/connector-ops team/extensibility type/enhancement New feature or request

Comments

@flash1293
Copy link
Contributor

flash1293 commented Dec 5, 2022

JSON schema is more flexible than what the current connector form UI can handle. However, it's currently not clearly documented what will work and what won't, so a connector developer is left with trial and error.

The SATs should check whether the spec is "UI-compliant" and the documentation should mention that test.

  • Only strings, integers and numbers can be secrets, arrays and objects do not work correctly in the UI. There was an existing related test already which would also accept objects and arrays - made it stricter to not accept them anymore
  • "type" can't be an array, except for the case where it's two values and the second one is "null" - all other cases are not handled by the UI
  • The array "items" type can't be an array
  • The array "items" type either needs to be unspecified which defaults to string, explicitly string, object, number, integer or has an enum defined (other cases won't work in UI)
  • not, anyOf, itemPrefixes, patternProperties, allOf, if, then, else, dependentSchemas and dependentRequired is not allowed as it won't be rendered as expected
  • If format: date/date-time is specified, it should have the corresponding pattern set as well and the other way around - this is only a warning as it's not a hard requirement and the UI still works (also enforcing this would require a lot of breaking changes)
  • An object or array type always needs a valid definition of the sub types (items in case of array, properties in case of object)
@flash1293 flash1293 added type/enhancement New feature or request area/frontend Related to the Airbyte webapp from/connector-ops labels Dec 5, 2022
@octavia-squidington-iv
Copy link

cc @airbytehq/frontend

@lmossman
Copy link
Contributor

lmossman commented Dec 8, 2022

@flash1293
Copy link
Contributor Author

flash1293 commented Jan 9, 2023

Some additional requirements copied over from #12330

We would it as one of the standard tests for each connectors so that before publishing a connector the developer could bring the spec into compliance.

So far the subset I have in mind is:

  • don't use not, anyOf, patternProperties or allOf.
  • don't use if/then/else, dependentSchemas or dependentRequired
  • type MUST be set. for pure JSONSchema there are reasons why you might want to leave it unset, but for our use case, doing that is just a headache. See the long conversation Benoit and I had in the linked issue.
  • the oneOfs stuff that is mentioned in the docs you linked. Done by SAT: Introduce new way to compare records based on PK #9718 #9768
  • When airbyte_secret can be used. I think the only types that should have airbyte_secret added to it are string, number, array<string> , and array<number>. But we have to do all of this other bonkers stuff right now to cover our behinds because theoretically someone could naively stick that flag anywhere. A compile time loud failure saves a lot of confusion at runtime. Done by #2576 oncall. SAT: test spec against exposed secrets #19124
  • Honestly saying that the type field for a spec must either be a value (i.e. type: string) or an array with 1 or 2 values. If 2 values, one of them must be null (i.e. type: [string] or type: [string, null]). Having to reason about type: [object, array, string] is very scary for secrets.
  • all of the possible values for a oneOfs should be of the same type. i.e. no oneOf: object, string, array. Again just super scary to deal with clearly especially for secrets. if you're using oneOf, let it be of a oneOf multiple objects and then nest whatever fields you need inside that object.

@flash1293
Copy link
Contributor Author

flash1293 commented Jan 16, 2023

Changes:

  • Only strings, integers and numbers can be secrets, arrays and objects do not work correctly in the UI
  • Fail on non_secrets hidden as they will cause weird behavior in the UI
  • "type" can't be an array, except for the case where it's two values and the second one is "null" - all other cases are not handled by the UI
  • The array "items" type can't be an array
  • The array "items" type either needs to be string, array or has an enum defined (just regular number won't work in UI)
  • type can either be a value directly or an array with two values with the second one being null (only the first one is respected by the UI)
  • not, anyOf, patternProperties, allOf, if, then, else, dependentSchemas and dependentRequired is not allowed as it won't be rendered as expected
  • If format: date/date-time is specified, it should have the corresponding pattern set as well
  • An object or array type always needs a valid definition of the sub types (items in case of array, properties in case of object)

@evantahler
Copy link
Contributor

evantahler commented Jan 30, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp from/connector-ops team/extensibility type/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants