🐛 [CDK, Declarative Source]: fix bug when type is missing for anyOf in nested arrays

[bazarnov]

What

Resolving:

• https://github.com/airbytehq/oncall/issues/5691

How

• added the check for missing type when there is nested anyOf with no type expected further
• covered the case with unit_test
• refactored the _clean method to make it easier to follow:
  • split the responsibilities of the single _clean method into multiple smaller ones, so we can narrow down the errors when they will occur next time (much easier to know the root cause of the issue with schema)
  • made reusable code parts, such as Literals to be re-usable (_NULL_TYPE, _TYPE, etc)

User Impact

No impact is expected.

Can this PR be safely reverted and rolled back?

• YES 💚
• NO ❌

[lmossman]

Somewhat tangential question: why is this checking that the length of node["anyOf"] is 2? What if there are three or more values in the anyOf? It seems like we'd want to handle that case here as well, right?

[girarda]

@lmossman do we allow for multiple types? My understanding is that len == 2 means there is a type + null

[lmossman]

In the OC issue that prompted this PR, the input data is

{
    "attributes": [
        {
            "title": "Usual Clothing Size",
            "type": "multi-value",
            "value": [
                "XL"
            ]
        },
        {
            "title": "Where do you live?",
            "type": "location",
            "value": {
                "countryCode": "GB",
                "countryName": "United Kingdom"
            }
        }
    ]
}

which is resulting in this inferred schema for the attributes property:

{
    "anyOf": [
        {
            "type": "array",
            "items": {
                "type": "string"
            }
        },
        {
            "type": "object",
            "properties": {
                "countryCode": {
                    "type": "string"
                },
                "countryName": {
                    "type": "string"
                }
            }
        }
    ]
}

so that is an example where we have an anyOf with one of the values not being type: null. So I could imagine a case where that value field had yet another value type in the data (e.g. a number) that would result in the inferred anyOf containing 3 different values

[girarda]

@lmossman is that even supported by destinations? My feeling is the connector's output should be consistent - in this case, emitting only arrays of strings, and wrapping the singleton strings as necessary.

CC @edgao do destinations support fields with multiple types (eg Union[str, List[str]])?

[edgao]

tl;dr I think this is fine, with some caveats

DV2 destinations current behavior:

• {oneOf: [...]} is translated to the destination's JSON type (jsonb/super/variant/etc). So this will work reasonably well, though the UX isn't the greatest
• {anyOf: [...]} will be treated as an "unrecognized" type, which also just defaults to the destination's JSON type. So it'll behave the same as oneOf, just for different reasons
• {type: [something_not_null, something_else_not_null, ...]} is handled using legacy logic ported from normalization (it picks the "widest" type amongst the options)
  • this logic is kind of dumb, e.g. type: [boolean, timestamp] becomes a timestamp
  • but even if it picks a dumb type, we'll just null out offending values and add an entry to airbyte_meta, so this doesn't strictly block syncs

in the distant future we'll probably (a) make all of those behave identically, and (b) pick the smallest type that can hold all of the unioned types. Not happening anytime soon though.

statically-typed file destinations (i.e. gcs/s3 in avro/parquet mode) already have handling for oneOf (... and we want to rewrite these destinations anyway).

references:

• DV2 JsonSchema parser https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/java/airbyte-cdk/typing-deduping/src/main/kotlin/io/airbyte/integrations/base/destination/typing_deduping/AirbyteType.kt#L25
• file destinations JsonSchema parser (we want to kill this and switch all destinations to use the DV2 schema parser, just fyi) https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/java/airbyte-cdk/s3-destinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/avro/JsonToAvroSchemaConverter.kt

[lmossman]

It feels strange to me to add the null type to the anyOf field, and then remove it at the end of this method.

It seems like it would make more sense to do one of the following:

1. Update the other checks to account for the anyOf case so that they allow the node to not have a type set, or
2. Add the type: [object, null] to the anyOf field and leave it there in the output schema

I'm not sure which approach is preferred. @girarda @maxi297 do either of you have an opinion here?

[girarda]

Always adding type: [object, null] feels ok to me since we implicitly allow all columns to be nullable

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 5, 2024 2:40pm

[bazarnov]

@edgao @girarda @lmossman @maxi297
I think we may proceed with what we have now; we should be fine, as far as this message explains how the destination works now.

Do have any action items to do here?

[edgao]

any reason you want to use anyOf instead of oneOf? (I mildly prefer oneOf for stricter semantics, but otherwise no concerns)

[bazarnov]

I don't think i have any preferences on this subject, it's more a matter of the fix, rather than logic change.

The AnyOf was before this issue have occurred, and it was working, should we hold this fix and refactor all of this logic in the way it fits the destination needs?

@girarda @edgao ?

[edgao]

your call, I don't know the LOE here, and I don't feel strongly 🤷 like my comment says - they'll both work, but oneOf is the case that we have explicit handling for. allOf is just falling into the backstop handler.

[girarda]

lgtm ✅

good scout work @bazarnov ! Thanks for the refactor into smaller functions to make the code easier to read