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

🪟🐛 [Connector Builder] Fix manifest conversion bugs #21828

Merged
merged 8 commits into from
Jan 26, 2023

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Jan 25, 2023

What

When testing out something unrelated, I discovered a few bugs with our YAML -> UI conversion logic, which are fixed in this PR.

How

Bugs fixed:

  • The CheckStream block was being generated with an empty set of stream_names, which caused errors for users when trying to use their generated YAML manifests, as described in [Connector Builder] Set check to the first stream by default #21612
    • Fix: Add logic to the convertToManifest method to filter down to the stream names in the builder form values checkStreams field which match the actual names of streams. If there are no matching stream names, it just sets it to the first stream (unless there are no streams, in which case it sets it to []. A connector with no streams is useless anyway so this should be fine).
      The manifestToBuilderForm code already sets the values.checkStreams field to what is set in the manifest;s CheckStream block, so this will use the set of valid stream names from the manifest if present.
  • OAuthAuthenticators could not be converted, because we were trying to compare each stream's manifest authenticator with the first stream's converted authenticator, meaning the refresh_request_body field was converted to an array in the latter but was an object in the former, so the equality check always failed.
    • Fix: use the first stream's manifest authenticator for comparison instead of its converted builder authenticator (also pass the first stream's url_base directly for consistency)
  • If an authenticator had no type set, we would encounter a runtime error trying to access the type.
    • Fix: add an explicit check for undefined type on authenticator
  • The OAuthAuthenticator's refresh_request_body can contain an object with nested objects/arrays, but the UI only allows key-value pairs for this field.
    • Fix: add an explicit check for nested objects/arrays in this field before converting, and throw an error if found
  • A user reported having a YAML file generated containing YAML anchors/refs:
    This is not desired, as it doesn't match the ref mechanism that is used in the low-code CDK, and it is not expected for us to be producing refs of any form in the YAMLs at the moment.
    • Fix (hopefully): set the noRefs option to true in the js-yaml dump() call that we make, which should prevent it from generating refs

I also added unit tests for the complex cases above

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 25, 2023
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, tested the cases and everything works really well! Just left one nit-pick which can be solved without another review.

The ref thing is interesting, I wasn't aware yaml would do such a thing, TIL.

} else if (manifestAuthenticator.type === "CustomAuthenticator") {
throw new ManifestCompatibilityError(streamName, "uses a CustomAuthenticator");
} else if (manifestAuthenticator.type === "OAuthAuthenticator") {
if (
Object.values(manifestAuthenticator.refresh_request_body ?? {}).filter(
(val) => (val !== null && typeof val === "object") || Array.isArray(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultra nit-picky - the only values we support are strings but this logic will let numbers and booleans through. The behavior is the following:

  • As long as you don't touch the input, the type is preserved
  • As soon as you type in the value field, it will implicitly cast to string

Seems cleaner to just throw on this until we actually support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I agree we should just throw on non-string values for now. Updated

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants