commit message change

connection resolver refactoring

doc updates

commit message update

type hints

refactoring

missing functions

rebase

rebase

review comments

rebase

review comments

address review comment

In D8511#220865, @vlorentz wrote:

Using .lower() on byte strings of unknown encoding is going to have surprising results.

What is the intended use of making this case-insensitive?

typo

add extra comment; update commit mesasge

change commit message

revert generic filters

add generic filter for connections

rebase

In D8462#219754, @anlambert wrote:

Looks good to me, just one missing return type to add.

Reading you code more deeply, I think it would be better to get rid of the resolvers_factory module
as it adds a not really needed indirection and makes code hard to follow.

Returning the proper node object directly in the *_resolver functions seems simpler to me and
you can set a local mapping based on object type in them when returning an Union.

add missing return; from review comments

improve tests from review comment

use a timestamp instead of seconds and microseconds

Add types and support for None date

removed a unit test

commit message update

return None for snapshot instead of an Object

In D8426#219120, @vlorentz wrote:

Here is a way to do it:

diff --git a/swh/graphql/resolvers/resolvers.py b/swh/graphql/resolvers/resolvers.py
index 60992bb..1b0e93f 100644
--- a/swh/graphql/resolvers/resolvers.py
+++ b/swh/graphql/resolvers/resolvers.py
@@ -93,6 +93,8 @@ def snapshot_resolver(
 def visit_snapshot_resolver(
     obj, info: GraphQLResolveInfo, **kw
 ) -> rs.snapshot.VisitSnapshotNode:
+    if obj.snapshotSWHID is None:
+        return None
     resolver = get_node_resolver("visit-snapshot")
     return resolver(obj, info, **kw)
 
diff --git a/swh/graphql/tests/functional/test_visit_status.py b/swh/graphql/tests/functional/test_visit_status.py
index a250a9a..d83861a 100644
--- a/swh/graphql/tests/functional/test_visit_status.py
+++ b/swh/graphql/tests/functional/test_visit_status.py
@@ -28,7 +28,7 @@ def test_get_visit_status(client):
         visit.origin,
         visit.visit,
     )
-    data, _ = get_query_response(client, query_str)
+    data, err = get_query_response(client, query_str)
     result_status = get_visit_status()[3]
     assert data["visit"]["status"]["nodes"][0] == {
         "date": result_status.date.isoformat(),
@@ -36,6 +36,7 @@ def test_get_visit_status(client):
         "status": result_status.status,
         "type": result_status.type,
     }
+    assert err is None
 
 
 def test_get_visit_missing_snapshot(client):
@@ -67,7 +68,4 @@ def test_get_visit_missing_snapshot(client):
         "status": result_status.status,
         "type": result_status.type,
     }
-    assert len(err) == 1
-    assert (
-        "Cannot return null for non-nullable field Snapshot.swhid" in err[0]["message"]
-    )
+    assert err is None

update error to raise

In D8426#219093, @vlorentz wrote:

Sure, but in that example, the error is because for some reason, they could not get the value of name for an object that exists. The error in the example even reflects this, as it mentions "character with ID 1002".

In our case, that object does not exist at all.

In D8426#219084, @vlorentz wrote:

The section you linked mentions null values in Non-Null fields, but not of null values for objects with a Non-Null field.

Are you sure this is disallowed by the standard, not "only" a bug in the GraphQL library we use?

In D8426#219084, @vlorentz wrote:

The section you linked mentions null values in Non-Null fields, but not of null values for objects with a Non-Null field.

Are you sure this is disallowed by the standard, not "only" a bug in the GraphQL library we use?

In D8426#219070, @vlorentz wrote:

In D8426#219051, @jayeshv wrote:

The issue is snapshot has non nullable fields (like SWHID) and GraphQL standard will not allow a
null for an object with non nullable field without an error.

I can't find what part of the standard disallows it, nor any mention one way or another outside the standard. Any pointer?

In D8426#219077, @anlambert wrote:

In D8426#219076, @jayeshv wrote:

> I do not get the issue here, using the following changes on top of your diff, it works as expected:
>

It will work as expected in the data part. But there will be an error associated, that we think is unnecessary.
With your changes, it will be a "NoneType' object has no attribute 'object_id" instead of an ObjectError

Could you write a snippet query to reproduce the error ? I still do not get what's wrong here.

> I do not get the issue here, using the following changes on top of your diff, it works as expected:
>

It will work as expected in the data part. But there will be an error associated, that we think is unnecessary.
With your changes, it will be a "NoneType' object has no attribute 'object_id" instead of an ObjectError

In D8426#219043, @vlorentz wrote:

Why have two different types, instead of making the snapshot field nullable, like this?

type VisitStatus {
  status: String!
  date: DateTime!
  type: String
  snapshot: Snapshot
}

@vlorentz The right fix is to add a new visit status type to the schema like below.

remove ununsed test