DR-3313: Add POST endpoint for retrieving data in order to enable larger filter queries in UI

[snf2ye]

Background

We have endpoints that enable retrieving and filtering on tabular data for a given dataset or snapshot. Before this change, these endpoints were GET endpoints, so there was a smaller limit to how big the filter statement could be. By switch these requests to be POST requests, we can support much larger filter statements.

The Changes

Mark existing GET endpoints as deprecated and "hidden" in swagger:

• lookupDatasetDataById
• lookupDatasetColumnStatisticsById
• lookupSnapshotPreviewById
  Create new endpoints with slightly new names to more accurately described how they are used:
• queryDatasetDataById
• queryDatasetColumnStatisticsById
• querySnapshotDataById

Testing

Changes were manually tested in the UI. More details in the UI PR description- DataBiosphere/jade-data-repo-ui#1540

Changes for both the UI and API are deployed to my dev instance: https://jade-sh.datarepo-dev.broadinstitute.org/

[samanehsan]

I wonder if this is still a problem now that this is not a query parameter 🤔

[snf2ye]

Great question - After testing this, it appears that it is still needed. If the user doesn't include direction in their request, then it will be null.

[snf2ye]

I've realized that there are some issues with how we're setting our default values. I'm going to have a follow-on PR to address this: #1541

[samanehsan]

This looks like a leftover from testing! (same with line 275)

[snf2ye]

I'll clean these up!

[okotsopoulos]

This looks good! I had a few small questions / suggestions.

Is there a plan or ticket for full removal of the deprecated endpoints? I know we theorize that TDR UI is the only programmatic caller.

[okotsopoulos]

Would it be worth it to change our @Hidden endpoints to redirect to their replacements? I'm not wording this super clearly, but something like this:

@Hidden
@Override
  public ResponseEntity<DatasetDataModel> lookupDatasetDataById(
      UUID id,
      String table,
      Integer offset,
      Integer limit,
      String sort,
      SqlSortDirection direction,
      String filter) {
      var queryDataRequest = <construct this from the raw inputs>;
      return queryDatasetDataById(id, table, queryDataRequest);
}

[snf2ye]

Definitely a reasonable path forward - I've gone back and forth here.
My main hesitancy here is that I have switched all of our tests to use the post endpoint. I think it's reasonable to move our testing to the new endpoints since we're marking these old endpoints as deprecated. But, this also means that I feel less confident in making changes to these methods since we don't have the automated testing on it.
What do you think?

[okotsopoulos]

Closing the loop after discussing in stand-up -- we agreed that maintaining unit test coverage of both old and new endpoints via parameterized tests would be good enough assurance, and we can leave integration / connected tests to use the new endpoints!

[snf2ye]

@okotsopoulos - Thanks! I think using the method source parameterized tests were a great idea. I've made this change and I've changed the lookup endpoints to reference the new query code in the controller.

[rjohanek]

I'm just wondering why the new APIs are post methods. It seems their functions and descriptions state they retrieve.

[nmalfroy]

That's an awesome question. There's a common misconception that POST is an operation that modifies state but it's really more of a catchall HTTP verb. For operations that fetch those can be GET or POST but the problem with GET (well, one of the problems with GET) is that it doesn't really allow for a request body (I mean you can, but a lot of clients flat out disallow it) because it's meant to just pass in all it needs in the URL (e.g. using path or query parameters). Some of the problems with that are that

• URLs are limited in length and so you very quickly run into issues of max request length
• It's super gross to model complex queries in URL parameters and much much easier in JSON bodies

So the idea here is moving the data fetch to a POST request so that we can pass in the full query parameters.

I'll note that this is a direction that a lot of even low level web REST APIs have moved to (e.g. Elasticsearch)

Hopefully this isn't too much of a novel :). This is a great topic for our Thursday design sessions!

[rjohanek]

Ah that makes sense thank you!

[okotsopoulos]

@rjohanek if you're interested, we took on this work in response to a specific user pain point with the old endpoints, in case you'd like to see a concrete example of how this played out! https://broadinstitute.slack.com/archives/CD4HBRFMG/p1698249337991919

[nmalfroy]

This looks great!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

84.6% Coverage
0.0% Duplication