building-envelope-data / api

API specification to exchange data about building envelopes
MIT License
3 stars 1 forks source link

Do not return `null` for non-nullable fields #275

Closed simon-wacker closed 2 years ago

simon-wacker commented 2 years ago

For example, when a query asks for the non-nullable field arguments of appliedMethod, the IGSDB returns null. If there are actually no arguments, it should just return an empty list. An executable example query is

query  {
  allOpticalData(
    where: {componentId : {equalTo: "6a6a0f0b-5b79-4167-841e-b3a4462a1d6e"}}
    )
  {
    nodes {
      uuid
      appliedMethod {
        methodId
        arguments {
          name
        }
      }
    }
  }
}

The same is true for warnings

query {
  allOpticalData(
    where: {componentId : {equalTo: "6a6a0f0b-5b79-4167-841e-b3a4462a1d6e"}}
    )
  {
    nodes {
      warnings
    }
  }
}

and creatorId, createdAt, appliedMethod.sources, locale, nonRootVertices, resourceTree.root.vertexId, resourceTree.root.value.archivedFilesMetaInformation, and resources. In your case, the field resources should be a singleton array with the one and only root resource.

StephenCzarnecki commented 2 years ago

@simon-wacker I don't think any of those fields have been handled in the IGSDB implementation yet because I do not believe they were part of what was discussed in #202.

There may also be other issues with some of them. E.g. from what I remember the IGSDB does not have information that corresponds to creatorId. Because I believe, but may be mistaken, that creatorId is supposed to refer to who created the data, i.e. the institution that did the actual measurements. And that information is not currently in the IGSDB.

So I am not sure how to address this issue at the moment, let me know if you have any ideas.

RDmitchell commented 2 years ago

If we don't have the fields in the IGSDB, they obviously can't be queried against.

RDmitchell commented 2 years ago

I believe these were not in the definition for MVP, so handling these seems outside of the current scope.

christoph-maurer commented 2 years ago

@RDmitchell @StephenCzarnecki I fully agree that we should not start now to implement that more data is returned, especially if this data is not available! I think @simon-wacker 's comment is technical: When IGSDB cannot return data for a field, please return an empty list instead of null. Is this easy to implement? I propose to prioritize it with Prio4,5 between #273 and #274.

StephenCzarnecki commented 2 years ago

@christoph-maurer To clarify: I do not believe any of the following are defined as lists in database.graphql: creatorId, createdAt, locale, or vertexId. Do you want IGSDB to return empty lists for those as well when there is no data?

simon-wacker commented 2 years ago

No, please do not return empty lists for fields that are not of type list. If you do not have a value for a non-null non-list field, then just return null although that makes the implementation not conform to the specification and leads to run-time errors on the consumer side.

A minor improvement would be to get a null result for the closest nullable field up the tree together with an error saying that there is no data for the non-nullable field with path ... or that the field cannot be queried because it is not implemented yet. According to https://graphql.org/learn/serving-over-http/#response a GraphQL response can include errors alongside data. That is tedious to implement though if the library you use does not support that functionality.

Another improvement would be if the GraphQL specification of your implementation only listed what is actually implemented. So, in your case the fields creatorId, createdAt, ... would just not be part of the specification of your endpoint. Then at least by comparing the official specification and the implemented one we could easily see the differences and we could check whether a specific query can be handled by the IGSDB or not.

simon-wacker commented 2 years ago

Addition: You could also return default values for creatorId, createdAt, and so forth with the correct type. Like the UUID 0000..., some date-time value (like the Unix epoch), and so forth. Then, at least consumers of your API do not run into type errors.

StephenCzarnecki commented 2 years ago

I have updated the IGSDB server to return either empty lists or default values for the fields discussed above. Now the user can make a query like

query {
  allOpticalData(
    where: { 
      nearnormalHemisphericalVisibleTransmittances: { 
        some: { 
          greaterThanOrEqualTo: 0.9
        }
      }
    }
  ) {
    nodes {
      uuid
      timestamp
      componentId
      name
      appliedMethod {
        methodId
        arguments {
          name
        }
        sources {
          name
        }        
      }
      resourceTree {
        root {
          value {
            description
            hashValue
            locator
            dataFormatId
            archivedFilesMetaInformation {
              path
              dataFormatId
            }
          }
          vertexId
        }
        nonRootVertices
        {
          vertexId
        }
      },
      resources {
        description
        hashValue
        locator
        dataFormatId
        archivedFilesMetaInformation {
          path
          dataFormatId
        }
      }
      nearnormalHemisphericalVisibleTransmittances
      nearnormalHemisphericalVisibleReflectances
      nearnormalHemisphericalSolarTransmittances
      nearnormalHemisphericalSolarReflectances
      infraredEmittances
      warnings
      creatorId
      createdAt
      locale
    }
  }
}

and get results that look like

{
  "data": {
    "allOpticalData": {
      "nodes": [
        {
          "uuid": "2b1f0bbf-46c5-4f22-af88-c9f6dd7c035b",
          "timestamp": "2021-07-09T04:57:32.663Z",
          "componentId": "a2fd41df-47cb-4a8b-8ed9-13f0b4f1292e",
          "name": "iplus Top 1.1T on Clearlite 5 mm",
          "appliedMethod": {
            "methodId": "35e98d58-9627-4bdf-bf9f-f265471c1f24",
            "arguments": [],
            "sources": []
          },
          "resourceTree": {
            "root": {
              "value": {
                "description": "IGSDB data",
                "hashValue": "174f6c6a4d35519df1b6cbc74ed063e1bcf8e084277c92f428d855ce049afbe6",
                "locator": "https://igsdb-icon.herokuapp.com/api/v1/products/9821/?json_format=buildingenvelopedata.org",
                "dataFormatId": "9ca9e8f5-94bf-4fdd-81e3-31a58d7ca708",
                "archivedFilesMetaInformation": []
              },
              "vertexId": "00000000-0000-0000-0000-000000000000"
            },
            "nonRootVertices": []
          },
          "resources": [
            {
              "description": "IGSDB data",
              "hashValue": "174f6c6a4d35519df1b6cbc74ed063e1bcf8e084277c92f428d855ce049afbe6",
              "locator": "https://igsdb-icon.herokuapp.com/api/v1/products/9821/?json_format=buildingenvelopedata.org",
              "dataFormatId": "9ca9e8f5-94bf-4fdd-81e3-31a58d7ca708",
              "archivedFilesMetaInformation": []
            }
          ],
          "nearnormalHemisphericalVisibleTransmittances": [
            0.904961
          ],
          "nearnormalHemisphericalVisibleReflectances": [
            0.04781192,
            0.0536334
          ],
          "nearnormalHemisphericalSolarTransmittances": [
            0.6316686
          ],
          "nearnormalHemisphericalSolarReflectances": [
            0.2854422,
            0.240099
          ],
          "infraredEmittances": [
            0.0382093414664268,
            0.841154813766479
          ],
          "warnings": [],
          "creatorId": "00000000-0000-0000-0000-000000000000",
          "createdAt": "2017-02-13T08:09:33+00:00",
          "locale": "en-US"
        },
...

If there are any other fields that are missed or if there is a problem with any of the above let me know.

simon-wacker commented 2 years ago

Thanks, works like a charm. One last thing: Could you return a value for the field id? Ideally, the same value returned by the field uuid.

StephenCzarnecki commented 2 years ago

@simon-wacker The server should now be able to return id. If I run

query {
  allOpticalData(
    where: { 
      nearnormalHemisphericalVisibleTransmittances: { 
        some: { 
          greaterThanOrEqualTo: 0.9
        }
      }
    }
  ) {
    nodes {
      id
      uuid
    }
  }
}

I get

{
  "data": {
    "allOpticalData": {
      "nodes": [
        {
          "id": "8c9d9c54-c509-42ad-93c2-a3251495a8f7",
          "uuid": "8c9d9c54-c509-42ad-93c2-a3251495a8f7"
        },
        {
          "id": "f0e60251-2372-4787-ab6d-007d7adc3c2c",
          "uuid": "f0e60251-2372-4787-ab6d-007d7adc3c2c"
        },
...
simon-wacker commented 2 years ago

Thank you! :-)

simon-wacker commented 2 years ago

Dear @StephenCzarnecki, there is at least one optical data set that returns null for id and warnings (and possible other fields also). Sorry for spotting this error so late. It can be seen by querying for

query AllOpticalData {
  allOpticalData(where: {}) {
    nodes {
      id
    }
  }
}

on https://igsdb-icon.herokuapp.com/icon_graphql/

Could you fix this on short notice?

StephenCzarnecki commented 2 years ago

@simon-wacker I have committed a fix that hopefully resolves this latest problem. You were correct that there were some products types where the resolver was not correctly setting the id, warning, and a few other fields. I believe that is fixed. https://igsdb-icon.herokuapp.com/icon_graphql/ can now run the query

query AllOpticalData {
  allOpticalData(where: {}) {
    nodes {
      id
      warnings
    }
  }
}

and get results that look like

{
  "data": {
    "allOpticalData": {
      "nodes": [
        {
          "id": "0d2be9fd-33a6-4bf9-9c4e-c14a5b480596",
          "warnings": []
        },
        {
          "id": "2539c9d9-727b-4960-915e-3b6f35fd43b7",
          "warnings": []
        },
        {
          "id": "afcf2f12-66f1-4305-9eff-02331594f8e1",
          "warnings": []
        },
etc...

I think that now the fields should be the same for all product types. What I've done could probably use a little refactoring but if you can confirm the current implementation suits your needs for the upcoming demos I will hold off on making any more changes until after they have been completed.

And if you find anything else that does not work before then please let me know and hopefully I can get it fixed in short order.

christoph-maurer commented 2 years ago

@StephenCzarnecki Thank you, it works fine!