-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add support for "null" in case of missing values and add integration test for GraphQL
#4767
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
base: main
Are you sure you want to change the base?
Conversation
counter = { path = "../../../examples/counter" } | ||
counter-no-graphql = { path = "../../../examples/counter-no-graphql" } | ||
create-and-call = { path = "./create-and-call" } | ||
graph-ql-queries = { path = "./graph-ql-queries" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: let's be consistent with the spelling graphql
.
|
||
// READ1 | ||
|
||
for (query, expected_response) in [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to use insta
here (with the snapshot identifier being the query).
> { | ||
pub key: K, | ||
pub value: V, | ||
pub value: Option<V>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really convinced that map entry values should always be optional. E.G. when iterating over sets of entries (perhaps with entries
with a filter other than keys
) we should just skip missing entries, so the value should be non-optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the object filters
does not implement any significant filter. You have only two options: either None
with all keys returned or the keys being selected.
Maybe, the filters
are meant to be extended to a larger feature. If so, I have not seen a description of it as a GitHub issue. But then, the real question is what actually would be possible as filters
:
- What we are considering is the
CollectionView
/ReentrantCollectionView
function. The input type I and view type V are generic. So, how exactly can we input a meaningful filter? - The page GraphQL search and filter gives some examples like
(a) => a.genre == genre
witha
of the output type. How could we implement a filtering function on the view? - It does not appear that the
async-graphql
library has any functionality for filtering. So, I do not know if we could implement filtering in a generic way without some major engineering effort.
I have three other reasons for returning null
in case of missing values:
- The change is putting the behavior of
CollectionView
/ReentransCollectionView
in line withMapView
. Right now theMapView
is returningnull
for missing keys. I think it is important to haveMapView<K,V>
as near toCollectionView<K,RegisterView<V>>
as possible. - The change of returning
null
on missing values is extending the functionality. It is not breaking anything. All of the tests that were passing are still passing. - In the page GraphQL search and filter the schema for the filtering is "albums(input: AlbumsInput): [Album]! " and so it does return
null
on missing keys.
All that being said, I do not see this work as a final work on GraphQL for linera-views
. For example, cursors, intervals, or similar are definitely needed. It is possible to add those functionalities since they operate in a generic way. But we have to be clear about what is realistic to implement in linera-views
.
GraphQL
Motivation
GraphQL is the canonical way to access Wasm application state on Linera. Therefore, it is of paramount
importance of having excellent support for GraphQL in the
linera-views
.The canonical way of handling missing values in a query is to return
null
. Ignoring it and pretending that it wasnot queried is not the recommended way. This is for example visible on GraphQL Search and Filter – How to search and filter results with GraphQL.
In this exposition, we see that the GraphQL schema types are
The meaning of that schema is that the function
album
returns something that may be null, and thatalbums
returns a list of entries that may benull
. This is because by default in GraphQL a variable can be nulled. If not it has a postfix of "!".The GraphQL standard does not say that if a request is failing to return null. However, if returning null is not allowed, then in case of a missing key, you should return null at a higher level. See GraphQL, Section 7.1.2. Therefore for our case, it is better to return a null value in case of missing key.
Proposal
The
Entry
is changed in order to have a value that is anOption<V>
. The change go surprisingly smoothly.Test Plan
The CI.
Due to the importance of GraphQL in Linera applications, a specific smart contract has been created for which we can test the
Release Plan
This changes are also relevant to the Game Of Life test.
Links
See above.