Page MenuHomeSoftware Heritage

Return none for missing snapshot in a visit status
ClosedPublic

Authored by jayeshv on Sep 8 2022, 4:58 PM.

Details

Summary

Snapshot can be null for a visit status.
Return none instead of a snapshot object in that case.

Resolves T4514

Diff Detail

Repository
rDGQL GraphQL API
Branch
missing-snapshot-bug
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 31441
Build 49183: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 49182: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D8426 (id=30380)

Rebasing onto 467e0b7e28...

Current branch diff-target is up to date.
Changes applied before test
commit 4ba54d838fedaa2ec74fc3f8f30683c10b7545b0
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Sep 8 16:53:39 2022 +0200

    Return sensible errors for missing snapshots in visit status
    
    Snapshot will be None for a visit status with status other then full.
    Return a sensible error when snapshot is requested from such types.
    
    Related to T4514

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/158/ for more details.

vlorentz added a subscriber: vlorentz.

Should this really be an error? Isn't it enough to set snapshot to null?

Should this really be an error? Isn't it enough to set snapshot to null?

Maybe not. Let me fix it that way

Should this really be an error? Isn't it enough to set snapshot to null?

There is another issue here. Snapshot SWHID is a non nullable field in the schema.
Returning None for a snapshot will result in a "GraphQLError: Cannot return null for non-nullable field Snapshot.swhid"
So, we can either return a snapshot with all the non-nullable fields or there must be an error associated.

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

type VisitStatusPartial {
  status: String!
  date: DateTime!
  type: String
}

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

A null snapshot in VisitStatusFull will cause an object not found error.
The issue with this approach is, it will force the client to use an extra check in the query.

I think they should have been two different objects in the swh-model as well, the existing design is not really type friendly.

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

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

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

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

That is the existing system.
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 think it will be a bad idea to remove non-nullable fields from the Sanpshot object.

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

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

That is the existing system.
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 think it will be a bad idea to remove non-nullable fields from the Sanpshot object.

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

13:40 $ git diff
diff --git a/swh/graphql/resolvers/visit_status.py b/swh/graphql/resolvers/visit_status.py
index c5d2fa7..672a617 100644
--- a/swh/graphql/resolvers/visit_status.py
+++ b/swh/graphql/resolvers/visit_status.py
@@ -21,7 +21,8 @@ class BaseVisitStatusNode(BaseNode):
     @property
     def snapshotSWHID(self):  # To support the schema naming convention
         if self._node.snapshot is None:
-            raise ObjectNotFoundError("Snapshot is not available for this visit status")
+            # raise ObjectNotFoundError("Snapshot is not available for this visit status")
+            return None
         return CoreSWHID(object_type=ObjectType.SNAPSHOT, object_id=self._node.snapshot)
 
 
diff --git a/swh/graphql/tests/functional/test_visit_status.py b/swh/graphql/tests/functional/test_visit_status.py
index 9e5a876..6d84c56 100644
--- a/swh/graphql/tests/functional/test_visit_status.py
+++ b/swh/graphql/tests/functional/test_visit_status.py
@@ -67,8 +67,3 @@ def test_get_visit_missing_snapshot(client):
         "status": result_status.status,
         "type": result_status.type,
     }
-    assert len(err) == 1
-    assert (
-        err[0]["message"]
-        == "Object error: Snapshot is not available for this visit status"
-    )

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?

> 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

> 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

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

Oh I did not see the associated task T4514, will hack from it then.

> 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.

query origins {  
  origins(first: 2) {
  	  nodes {
        url
        visits(first: 2) {
          nodes {
            id
            status(first: 2) {
              nodes {
                status
                snapshot {
                  swhid
                }
              }
            }
          }
        }
      }
  }
}

run this in staging, you will get a weird error in the errors section.
Apply my patch and run using ''make run-wsgi" (that will connect to staging db), you will get a better error :).

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?

The SWHID is a non nullable scalar from interface MerkleNode that snapshot imeplements.

This is not specific, but an error is mandatory in case there is a missing non-nullable field.
https://spec.graphql.org/June2018/#sec-Errors

The error happens only when SWHID or ID, both non-nullable, are requested from a snapshot
for eg flowing query will not cause an error. (It will cause an error in the staging as it configured to raise in case the parent is missing)

query origins {  
  origins(first: 1) {
  	  nodes {
        url
        visits(first: 1) {
          nodes {
            id
            status(first: 2) {
              nodes {
                
                status
                snapshot {
                  branches(first: 1) {
                    nodes {
                      name {
                        text
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
  }
}

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?

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?

I think it is the right behavior. Let me test with another library to make sure.

It gets confusing.The schema validation allows to return a null object with non-nullable fields :)

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?

@vlorentz I think the behavior is correct, but the error we report is not.
http://spec.graphql.org/June2018/#example-08b62

read the block above this
"""
If the field which experienced an error was declared as Non-Null, the null result will bubble up to the next nullable field. In that case, the path for the error should include the full path to the result field where the error occurred, even if that field is not present in the response.
"""
So, we should just return None for snapshot and should not raise an error.
There will be agraphql error with entire path to the non nullable field. (two errors in case both id and swhid are in the request)

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.

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.

Correct. In our case, SWHID is null and that will anyway make the snapshot null. But SWHID is null because snapshot is already null :).
Maybe we should have an error like below, what do you think?

"errors": [
    {
      "message": "Snapshot.swhid can not be fetched as the snapshot is null",
      "locations": [
        {
          "line": 12,
          "column": 17
        }
      ],
      "path": [
        "origins",
        "nodes",
        0,
        "visits",
        "nodes",
        0,
        "status",
        "nodes",
        0,
        "snapshot",
        "swhid"
      ],

Build is green

Patch application report for D8426 (id=30418)

Rebasing onto 6e5d34e94d...

Current branch diff-target is up to date.
Changes applied before test
commit f34bcebcfd457dee3f980c4462018d20238669de
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Sep 8 16:53:39 2022 +0200

    Return sensible errors for missing snapshots in visit status
    
    Snapshot will be None for a visit status with status other then full.
    Return a sensible error when snapshot is requested from such types.
    
    Related to T4514

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/165/ for more details.

Correct. In our case, SWHID is null and that will anyway make the snapshot null. But SWHID is null because snapshot is already null :).

Ah, I see. But that we have a null SWHID is an implementation detail, because the SWHID is how the snapshot is retrieved. If it was any other field, we would not have an error.

From the protocol's point of view, this implementation matter should not be visible.

Maybe we should have an error like below, what do you think?

There should not be an error at all, because it is a completely expected condition.

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

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

This makes sense, Thanks.

return None for snapshot instead of an Object

Build is green

Patch application report for D8426 (id=30424)

Rebasing onto 6e5d34e94d...

Current branch diff-target is up to date.
Changes applied before test
commit 9b8c73df54d4b7bfe7df3d1f795dcd96ceba7857
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Sep 8 16:53:39 2022 +0200

    Return sensible errors for missing snapshots in visit status
    
    Snapshot will be None for a visit status with status other then full.
    Return a sensible error when snapshot is requested from such types.
    
    Related to T4514

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/166/ for more details.

jayeshv retitled this revision from Return sensible errors for missing snapshots in visit status to Return none for missing snapshot in a visit status.Sep 9 2022, 5:34 PM
jayeshv edited the summary of this revision. (Show Details)

Build is green

Patch application report for D8426 (id=30425)

Rebasing onto 6e5d34e94d...

Current branch diff-target is up to date.
Changes applied before test
commit 39c64ad55702142f90220f6254259f06d2fe948c
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Sep 8 16:53:39 2022 +0200

    Return none for missing snapshot in a visit status
    
    Snapshot can be null for a visit status.
    Return none instead of a snapshot object in that case.
    
    Resolves T4514

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/167/ for more details.

swh/graphql/tests/unit/resolvers/test_resolvers.py
30

why this change?

Build is green

Patch application report for D8426 (id=30434)

Rebasing onto 6e5d34e94d...

Current branch diff-target is up to date.
Changes applied before test
commit e2f9e03f3b242c6f22e3596432742876920d3434
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Sep 8 16:53:39 2022 +0200

    Return none for missing snapshot in a visit status
    
    Snapshot can be null for a visit status.
    Return none instead of a snapshot object in that case.
    
    Resolves T4514

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/168/ for more details.

jayeshv added inline comments.
swh/graphql/tests/unit/resolvers/test_resolvers.py
30

This is a generic test to make sure the correct resolver is called. It is assuming obj (the parent object) can be None. For visit_snapshot the assumption is not correct any more.
This test can be removed as it is already covered in functional tests.

swh/graphql/tests/functional/test_visit_status.py
10–70

Both tests are quite similar, you should merge them into a single one using pytest parametrize decorator:

@pytest.mark.parametrize(
    "visit, visit_status", list(zip(get_visits(), get_visit_status()))
)
def test_get_visit_status(client, visit, visit_status):
    query_str = """
    {
      visit(originUrl: "%s", visitId: %s) {
        status(first: 3) {
          nodes {
            status
            date
            type
            snapshot {
              swhid
            }
          }
        }
      }
    }
    """ % (
        visit.origin,
        visit.visit,
    )
    data, err = get_query_response(client, query_str)
    assert data["visit"]["status"]["nodes"][0] == {
        "date": visit_status.date.isoformat(),
        "snapshot": {"swhid": f"swh:1:snp:{visit_status.snapshot.hex()}"}
        if visit_status.snapshot is not None
        else None,
        "status": visit_status.status,
        "type": visit_status.type,
    }
    assert err is None
jayeshv marked an inline comment as done.

improve tests from review comment

This revision is now accepted and ready to land.Sep 12 2022, 2:32 PM

Build is green

Patch application report for D8426 (id=30450)

Rebasing onto 6e5d34e94d...

Current branch diff-target is up to date.
Changes applied before test
commit 2c3b16f7952be753ac80f115619109e2c41a514b
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Sep 8 16:53:39 2022 +0200

    Return none for missing snapshot in a visit status
    
    Snapshot can be null for a visit status.
    Return none instead of a snapshot object in that case.
    
    Resolves T4514

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/171/ for more details.