SAT: Relax checking of oneOf common property and allow optional default keyword additional to const keyword

[grubberr]

Signed-off-by: Sergey Chvalyuk grubberr@gmail.com

What

We have requirement for all oneOf common properties to use only const keyword without default and enum keywords like this:

"option_title": {
  "type": "string",
  "const": "OAuth Credentials"
}

"option_title": {
  "type": "string",
  "const": "PAT Credentials",
}

BUT new pydentic==1.10.0 after this PR pydantic/pydantic#4152
started to generate json-schemas with default keyword like this:

"option_title": {
  "type": "string",
  "const": "OAuth Credentials",
  "default": "OAuth Credentials"
}

"option_title": {
  "type": "string",
  "const": "PAT Credentials",
  "default": "PAT Credentials",
}

How

I propose to be compatible with pydantic>=1.10.0 and not forbid default keyword in such common properties.

[sherifnada]

@grubberr can we skip the pydantic major version upgrade for now? I don’t know the impact of allowing default on OneOf in the UI

[grubberr]

I have put this PR on-hold until we understand how to be with new pydantic==1.10.0
and default keyword in common oneOf property

[sherifnada]

@grubberr is there a way to modify pydantic behavior instead? It seems like the wrong choice to modify all of the Airbyte Protocol expectations because a python library changed an implementation detail.

My understanding is that this will cause a suboptimal UI experience. Randomly selecting @tealjulia to confirm. Would adding the enum keyword to the oneOf blocks cause issues in the Ui?

[grubberr]

/test connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/3000543214
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/3000543214
No Python unittests run

Build Passed

Test summary info:

All Passed

[grubberr]

@sherifnada I have double checked pydentic PR(s) and Issue(s) about that problem with const:

pydantic/pydantic#4031
pydantic/pydantic#4152

I see that pydentic team thinks that old behaviour (without default) was incorrect and they just fixed old behaviour.
I don't see a way to change it (at least not hacking library)

[teallarson]

Is my understanding correct that this should not change any existing frontend behavior and we'd continue reading the same keyword in the spec?

@grubberr @sherifnada If that is the case I believe this will not cause issues. The frontend types for connectors trust what the connector sends us and this appears to be solely an additive change. I tested it out locally in Storybook (our frontend component library) and the form is still able to at least render with the additional keyword in the spec.

I strongly recommend this be manually tested in the UI to ensure that all of the user interaction still works as expected.

This could be achieved by:

1. changing a connector to use this format
2. running OSS against that connector and trying it out in the UI

I have not done this as I'm only sure how to do step 1 (would like to know how to do step 2, if someone can point me in the right direction I can try it out).

Another conservative measure would be to add a test to the frontend code where we parse the specs to ensure that it handles a spec with this keyword as expected (which, if I understand correctly, we should just ignore?). Happy to help get that in place.

I agree, I'd really rather be safe than sorry in ensuring users can setup their connectors.

[sherifnada]

@grubberr is there a way to override this behavior in Pydantic? If not, let's follow Teal's advice above:

1. manually test in the UI
2. create an issue for the frontend team asking them to add tests to validate this is safe to do

[grubberr]

@sherifnada
Hello
I only can repeat my last message
#16172 (comment)

[sherifnada]

@grubberr sounds fine with me, we'll need to follow the steps above to move this change forward