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

Nullable GraphQL API #1829

Merged
merged 2 commits into from Jun 20, 2022
Merged

Nullable GraphQL API #1829

merged 2 commits into from Jun 20, 2022

Conversation

benjaminpkane
Copy link
Contributor

Changes all run fields except key to be nullable for backward compatibility with dataset that have yet to be migrated. These datasets are still exposed in the GraphQL API for pagination.

Resolves #1806

Changes all run graphql fields except key to be nullable for backward
compat safety
@benjaminpkane benjaminpkane added bug Bug fixes app Issues related to App features server Issues related to server features or changes labels Jun 6, 2022
@benjaminpkane benjaminpkane requested a review from a team June 6, 2022 15:21
@benjaminpkane benjaminpkane self-assigned this Jun 6, 2022
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

I confirmed that I'm now able to load my particular older datasets (created prior to v0.14 and currently migrated to v0.15.1), but isn't this treating a symptom and not the problem? Other old datasets will surely cause some other errors...

It seems to me that, since we currently only lazily migrate datasets to the current version when they are loaded, the App must throw a huge try-except around anything related to listing datasets and just say "well, I guess I can't include that dataset in my list until the user migrates it".

@brimoor
Copy link
Contributor

brimoor commented Jun 7, 2022

The lazy-dataset-migration design will obviously continue to cause issues for all features that are supposed to work across datasets. Probably guarantees we'll change this model in Teams.

But, if the OSS App remains a purely single dataset visualization tool, then the dataset list dropdown seems like the only problem area and thus a "patch" could make sense.

@benjaminpkane
Copy link
Contributor Author

I made all fields except name optional which accounts for the progression of the dataset document from prior versions.

Generally, GraphQL intentionally separates the internal representation from the exposed API, so backward compatibility comes naturally. If we need to account for changes, we just bake it into the resolvers.

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

LGTM

@benjaminpkane benjaminpkane merged commit 62b8030 into develop Jun 20, 2022
@benjaminpkane benjaminpkane deleted the bug-1806 branch June 20, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Issues related to App features bug Bug fixes server Issues related to server features or changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] App can't be opened when deployment contains datasets with no creation date
3 participants