Return none for missing snapshot in a visit status

Build is green

Patch application report for D8426 (id=30380)

Rebasing onto 467e0b7e...

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 #4514

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

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

! In !54 (closed), @vlorentz wrote: Should this really be an error? Isn't it enough to set snapshot to null?

Maybe not. Let me fix it that way

! In !54 (closed), @vlorentz wrote: 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
}

! In !54 (closed), @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
}

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.

! In !54 (closed), @jayeshv wrote:

! In !54 (closed), @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
}
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"
-    )

! In !54 (closed), @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?

> 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 !54 (closed), @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.

! In !54 (closed), @anlambert wrote:

! In !54 (closed), @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.

Oh I did not see the associated task #4514 (closed), will hack from it then.

! In !54 (closed), @anlambert wrote:

! In !54 (closed), @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.

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

! In !54 (closed), @vlorentz wrote:

! In !54 (closed), @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?

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?

! In !54 (closed), @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?

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 :)

! In !54 (closed), @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?

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

! In !54 (closed), @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.

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"
      ],

update error to raise

Return none for missing snapshot in a visit status

Merge request reports

Activity

Patch application report for D8426 (id=30380)

Changes applied before test