Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination metadata #51

Merged
merged 14 commits into from
Jun 30, 2023
Merged

Pagination metadata #51

merged 14 commits into from
Jun 30, 2023

Conversation

alancleary
Copy link
Contributor

This PR adds support for pagination metadata to the GraphQL API and moves the API to page-based pagination. The major changes it introduces to achieve this are:

  1. All Intermine data-source API methods now return an ApiResponse type that allows the requested results to be returned along with arbitrary metadata.
  2. All Intermine data-source API methods that return multiple elements now make two requests: the original pathQuery request to get the data and a new summaryPath request to get pagination info, which is returned as metadata using the ApiResponse type
  3. All top-level (i.e. entrypoint) GraphQL fields have been updated to return a Results type with the requested data being located in the types' results field. The Results types for top-level fields that support pagination also have a pageInfo field of type PageInfo, which contains pagination information.
  4. All top-level fields and related code have been updated to use the page and pageSize parameters for page-based pagination, replacing the start and size parameters previously used for offset pagination. InterMine only supports offset pagination so a shim has been added to the Intermine data-source to convert page-based parameters to offset parameters before sending requests.

During testing I found many subfields in our API weren't implemented and not all annotatable, bio entity, and sequence feature types were consistent in their implementations. This PR fixes these issues because fixing a lot of it was required to test the previously mentioned changes so I went ahead and fixed it all.

@sammyjava I tried to be thorough with my testing but I wouldn't be surprised if something slipped by. Please put this PR through the ringer before approving!

This is to support associating arbitrary metadata with the results of any top-level query, such as pagination information.
This type allows both data and metadata to be returned by a n InterMine data source API call. Subsequently, an InterMineSummaryResponse type was introduced for providing PageInfo objects in a requests response, and the existing Rersponse type was renamed IntermineDataResponse to better distinguish it from the new types. The existing InterMine data source models were updated to use these types.
…o also return pagination information.

This includes adding pagination support to some APIs for consistency. These changes required updating several subfield resolvers. Note that our API current doesn't support pagination of subfields so the affected resolvers simply omit the pagination information from their output.
… support metadata.

Affected (sub)field resolvers were also updated.
This includes updating the annotatable, bio entity, and sequence feature interfaces and making sure all such types use the interfaces. Subsequently, corresponding subfield resolver factories were introduced to ensure these types' subfield resolvers are consistent and correct. Some InterMine types were updated to be consistent with the GraphQL types and themselves. Lastly, some subfield resolvers that were simply not implemented were added.
InterMine uses offset pagination so new variables were used to maintain support for InterMine's offset pagination, athough page-based pagination is now all that's available via the GraphQL API.
@alancleary alancleary requested a review from sammyjava June 27, 2023 21:41
@sammyjava
Copy link
Contributor

sammyjava commented Jun 28, 2023

query:

query Genes($page: Int, $pageSize: Int, $genus: String) {
  genes(page: $page, pageSize: $pageSize, genus: $genus) {
    pageInfo {
      currentPage
      hasNextPage
      hasPreviousPage
      numResults
      pageCount
      pageSize
    }
    results {
      dataSets {
        name
      }
    }
  }
}

returns error on null dataSets (which is legit, genes lack dataSets collection):

{
  "data": {
    "genes": null
  },
  "errors": [
    {
      "message": "Cannot return null for non-nullable field Gene.dataSets.",
      "locations": [
        {
          "line": 12,
          "column": 7
        }
      ],
      "path": [
        "genes",
        "results",
        0,
        "dataSets"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "stacktrace": [
          "Error: Cannot return null for non-nullable field Gene.dataSets.",
          "    at completeValue (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:594:13)",
          "    at executeField (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:489:19)",
          "    at executeFields (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:413:20)",
          "    at completeObjectValue (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:914:10)",
          "    at completeValue (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:635:12)",
          "    at completeValue (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:584:23)",
          "    at /home/shokin/graphql-server/node_modules/graphql/execution/execute.js:696:25",
          "    at Function.from (<anonymous>)",
          "    at completeListValue (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:676:34)",
          "    at completeValue (/home/shokin/graphql-server/node_modules/graphql/execution/execute.js:607:12)"
        ]
      }
    }
  ]
}

@sammyjava
Copy link
Contributor

query Genes($page: Int, $pageSize: Int, $genus: String) {
  genes(page: $page, pageSize: $pageSize, genus: $genus) {
    pageInfo {
      currentPage
      hasNextPage
      hasPreviousPage
      numResults
      pageCount
      pageSize
    }
    results {
      organism {
        genus
        species
      }
      identifier
    }
  }
}

returns null organism (I checked that the gene does in fact have an organism reference):

{
  "data": {
    "genes": {
      "pageInfo": {
        "currentPage": 1363,
        "hasNextPage": false,
        "hasPreviousPage": true,
        "numResults": 27243,
        "pageCount": 1363,
        "pageSize": 20
      },
      "results": [
        {
          "organism": null,
          "identifier": "phavu.G19833.gnm1.ann1.Phvul.L011300"
        },
        {
          "organism": null,
          "identifier": "phavu.G19833.gnm1.ann1.Phvul.L011400"
        },
        {
          "organism": null,
          "identifier": "phavu.G19833.gnm1.ann1.Phvul.L011500"
        }
      ]
    }
  }
}

@alancleary
Copy link
Contributor Author

I'm surprised that one slipped by; I especially scrutinized the annotatable, bio entity, and sequence feature code. Anyway, the gene resolver was using the annotatable subfield factory instead of the sequence feature subfield factory. A simple fix!

@sammyjava
Copy link
Contributor

Let's hang on those two and then I'll do more tests after the fixing commit.

@alancleary
Copy link
Contributor Author

Let's hang on those two and then I'll do more tests after the fixing commit.

The null organism was the same issue as the null data sets; I coded the gene resolver as an annotatable instead of as a sequence feature. I've confirmed both queries are working now.

@sammyjava
Copy link
Contributor

Sorry, I was looking at the wrong mine, that gene did have dataSets, and you've fixed it.

@sammyjava
Copy link
Contributor

Should I double-check all the Annotatables, BioEntities, and SequenceFeature implementations to catch others of this issue? Or do you think you have them all now?

@alancleary
Copy link
Contributor Author

Should I double-check all the Annotatables, BioEntities, and SequenceFeature implementations to catch others of this issue? Or do you think you have them all now?

I thought I double checked all of them but apparently I missed genes. I haven't checked to see if the others are correct since opening the PR. Maybe just check them via queries in Apollo Explorer?

What other issues were you referring to during our Zoom call? The only one that was clear to me is that the sequence feature resolver factory is currently just a pass-through to the bioentity factory. This is because currently none of the sequence feature types implement attributes beyond what's inherited from bio entity. Per our discussion in issue #43, my intent here is to only implement missing resolvers for subfields already defined in our types. But since we'll probably expand the API to include sequence feature specific attributes in the future, I added the resolver now (even though it's just a pass-through) so all we have to do to implement the subfield resolvers for these attributes is update the sequence feature resolver factory instead of tracking down and updating the resolvers of every sequence feature type. TL;DR: I'm avoiding technical debt by using a pass-through resolver factory for sequence feature types.

@sammyjava
Copy link
Contributor

Yup, sorry - I forgot that don't have SequenceFeature.chromosomeLocation and SequenceFeature.supercontigLocation in favor of BioEntity.locations, where Location has Location.chromosome and Location.supercontig. (That's actually a kind of funky thing in InterMine, but for some reason they wanted to directly reference the chromosome in SequenceFeature, possibly a legacy carryover in the core data model. I added supercontig.)

I'll hit some more testing tomorrow AM.

Copy link
Contributor

@sammyjava sammyjava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not finding any issues at this point. Approving.

src/resolvers/intermine/bio-entity.ts Show resolved Hide resolved
@alancleary alancleary merged commit 38b85f6 into main Jun 30, 2023
@alancleary alancleary deleted the pagination-metadata branch July 19, 2023 20:47
@alancleary alancleary restored the pagination-metadata branch July 19, 2023 20:47
@alancleary alancleary deleted the pagination-metadata branch July 19, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants