⚙ D8426 Return none for missing snapshot in a visit status

jayeshv created this revision.Sep 8 2022, 4:58 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 8 2022, 4:58 PM

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.

Harbormaster completed remote builds in B31400: Diff 30380.Sep 8 2022, 5:03 PM

jayeshv requested review of this revision.Sep 8 2022, 5:03 PM

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

In D8426#218850, @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 D8426#218850, @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 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
}

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 D8426#219051, @jayeshv wrote:
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
}
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 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?

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

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.

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

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.

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

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

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

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

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

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.

Harbormaster completed remote builds in B31435: Diff 30418.Sep 9 2022, 3:55 PM

In D8426#219097, @jayeshv wrote:

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

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

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.

Harbormaster completed remote builds in B31441: Diff 30424.Sep 9 2022, 5:20 PM

commit message update

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.

Harbormaster completed remote builds in B31442: Diff 30425.Sep 9 2022, 5:36 PM

vlorentz added inline comments.Sep 12 2022, 10:34 AM

swh/graphql/tests/unit/resolvers/test_resolvers.py
30	why this change?

removed a unit test

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.

Harbormaster completed remote builds in B31450: Diff 30434.Sep 12 2022, 10:52 AM

jayeshv marked an inline comment as done.Sep 12 2022, 10:53 AM

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.

anlambert added inline comments.Sep 12 2022, 1:35 PM

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

improve tests from review comment

jayeshv marked an inline comment as done.Sep 12 2022, 2:29 PM

Looks good to me.

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.

Harbormaster completed remote builds in B31460: Diff 30450.Sep 12 2022, 2:33 PM

Closed by commit rDGQL2c3b16f7952b: Return none for missing snapshot in a visit status (authored by jayeshv). · Explain WhySep 12 2022, 2:37 PM

This revision was automatically updated to reflect the committed changes.

jayeshv added a commit: rDGQL2c3b16f7952b: Return none for missing snapshot in a visit status.

Return none for missing snapshot in a visit status
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D8426 (id=30380)

Changes applied before test

Patch application report for D8426 (id=30418)

Changes applied before test

Patch application report for D8426 (id=30424)

Changes applied before test

Patch application report for D8426 (id=30425)

Changes applied before test

Patch application report for D8426 (id=30434)

Changes applied before test

Patch application report for D8426 (id=30450)

Changes applied before test

Revision Contents
Changeset List

Diff 30424

swh/graphql/resolvers/resolvers.py

swh/graphql/resolvers/snapshot.py

swh/graphql/resolvers/visit_status.py

swh/graphql/tests/data.py

swh/graphql/tests/functional/test_visit_status.py

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

Return none for missing snapshot in a visit statusClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D8426 (id=30380)

Changes applied before test

Patch application report for D8426 (id=30418)

Changes applied before test

Patch application report for D8426 (id=30424)

Changes applied before test

Patch application report for D8426 (id=30425)

Changes applied before test

Patch application report for D8426 (id=30434)

Changes applied before test

Patch application report for D8426 (id=30450)

Changes applied before test

Revision ContentsChangeset List

Diff 30424

swh/graphql/resolvers/resolvers.py

swh/graphql/resolvers/snapshot.py

swh/graphql/resolvers/visit_status.py

swh/graphql/tests/data.py

swh/graphql/tests/functional/test_visit_status.py

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

Return none for missing snapshot in a visit status
ClosedPublic
Actions

Revision Contents
Changeset List