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

fix: various nullable resources in OpenAPI config #12260

Merged
merged 1 commit into from
Apr 29, 2022
Merged

fix: various nullable resources in OpenAPI config #12260

merged 1 commit into from
Apr 29, 2022

Conversation

azhard
Copy link
Contributor

@azhard azhard commented Apr 21, 2022

What

Fixes a few instances of the problem mentioned in #10774 by setting the OpenAPI component schemas as nullable: true

Specifically adds nullability to the following:

  • CustomerioNotificationConfiguration
  • OperatorDbt
  • AttemptStats
  • AttemptFailureSummary
  • ResourceRequirements

How

Simply add nullable: true to the base component so that any operation that uses the component accepts the null version.

Recommended reading order

N/A

🚨 User Impact 🚨

Will allow users to better rely on generated clients from the Airbyte OpenAPI config - no breaking changes.

Pre-merge Checklist

N/A

@CLAassistant
Copy link

CLAassistant commented Apr 21, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/api Related to the api area/platform issues related to the platform labels Apr 21, 2022
@alafanechere alafanechere self-assigned this Apr 27, 2022
@alafanechere
Copy link
Contributor

Hey @azhard thank you for this PR.
For other reading it here's a bit of background context:

  • When our server returns non-required field, some of these fields have a null value.
  • Generated API client will fail validating responses if a field contains a null value and this field is not declared as nullable in the OpenAPI spec.

@cgardens suggested that we change the server behavior to make sure the response from the server does not contain the field that are not required if they are null. I'm not familiar enough with how the backend works to suggest this change myself but would be happy to get some hints on where the change should happen.

This change will prevent this kind of PR and the need to declare a lot of fields as nullable in our open API spec.

@azhard
Copy link
Contributor Author

azhard commented Apr 27, 2022

Hey @alafanechere, my strong preference for this would be to handle this from the server side and I'd also be more than happy to submit a PR to fix that if pointed in the right direction but like you not familiar enough with that code to make the changes.

So for now, this feels like a band-aid solution but still a solution at least.

@pmossman
Copy link
Contributor

@azhard I can try to point you in the right direction for potentially fixing this on the server side. I think your best bet would be to search for places in code where we construct generated API response objects that include the OpenAPI components you've identified, and then see if we can add handling for cases where the OpenAPI component is null.

For example, I'll try to walk through this process for AttemptFailureSummary:

First of all, note that I'm searching in my local project because the generated code in question isn't checked in to Github.

  1. If I search the generated code directory for @Valid AttemptFailureSummary, I can quickly see that our AttemptRead object is where I want to add a null check:

image

  1. If I search for wherever we construct new AttemptRead instances, I can quickly see that JobConverter.java is likely where I'll need to add a null check. In fact, I highlighted the exact line in the screenshot where we are likely setting the "failureSummary" to null in some cases:

image

  1. So, I'd probably need to modify that getAttemptRead method to only set the .failureSummary on the new AttemptRead() when the return value of getAttemptFailureSummary(attempt) isn't null.

I hope that is useful, I think this approach should be able to guide you to make necessary updates for all of the cases you identified. Let me know if you have any follow up questions!

@alafanechere alafanechere temporarily deployed to more-secrets April 29, 2022 09:02 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets April 29, 2022 09:02 Inactive
@alafanechere
Copy link
Contributor

@azhard I'll merge this to unblock you but please have a look at @lmossman explanation above if you encounter other similar problems.

@cgardens
Copy link
Contributor

This is a dangerous bandaid. It i setting a bad precedent that it is okay to compromise the API interface. We need to make sure we do the right fix before it gets exploited.

@alafanechere
Copy link
Contributor

@cgardens and @pmossman I opened this PR to try avoid this kind of bandaid in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/platform issues related to the platform community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants