Add Fields/Select Param for GET Endpoints

[seanmalbert]

There is a need to specify which columns we want selected and returned for an endpoint. The main /listings endpoint for public for example fetches every field and relation specified in the listing service, which is quite a lot, regardless if it's needed and thus has huge performance implications.

There are of course a number of ways to handle this, so I will start with one idea and we can go from there.

Using swagger's default style: form and not default explode: false for serialization, a simple query would look like:

/listings/?fields=id,name,applicationDueDate

That's pretty straight forward, but what about specifying relationships?

Since we can use . in query parameters this makes it easier:

/listings/?fields=id,name,units.id,units.unitType.id,units.unitType.name

Notice that in this example, the id is specified in the units and units.unitType relationships. Since a field like id is typically required for a join, we can either make it part of our documentation that those fields must also be selected, if you wish to have them returned or we can programmatically add them in if it's not present. For this idea, I think it makes sense to start with the simpler documentation approach.

Now to the more complicated side, actually parsing these to add the fields to the query builder. We could create a simple map that relates the fields that come through the parameter to what needs to get joined and selected, or we could handle it more generically. Below are my initial thoughts on how we could do this.

We'll want a function that takes the fields param, query builder (qb) to add the fields to and the default schema, so we can properly add the fields that belong to the entity.

function addFields(fields: string, qb: SelectQueryBuilder, schema: string): void {...}

Within addFields, we fields.split(",") and loop over it. If the field does not contain a ., then we can assume that field is on the model, so for example:

const testFieldRegex = new RegExp(/\./)
fieldArray.forEach(field => {
  if (testFieldRegex.test(field) === false) {
    qb.addSelect(`${schema}.${field}`)
  }
})

If testFieldRegex.test(field) === true, then we can call another function which traverses the field path, like ["units", "unitType", "name"]. This function will receive the field path, the schema of the field, query builder (qb) and a joins arg, so we can keep track of the joins that have already been added.

function joinAndSelect(path: string[], schema: string, qb: SelectQueryBuilder, joins: { [key: string]: boolean }): void {...}

We can first get the field's schema and the field with

const [relation, ...fields] = path

This way, if the field is nested like units.unitTypes.name, we can check if joins[relation] is undefined, if so, we can set that and add the join with

qb.leftJoin(`${schema}.${relation}`, relation)

In this example, since fields length is greater than one, we would pass fields as the path and relation as the schema back into joinAndSelect. If the fields length === 1, then we can call addSelect.

If the fields param is not present, then it makes sense to default to what we're doing now.

[seanmalbert]

@pbn4 and @emilyjablonski , can you review and provide feedback please. @willrlin , I'm curious as to how this or something like this may fit into your team's plans.

DHALIA handled this problem on the main listing view by adding a separate endpoint with fewer selects and joins. That is a possibility, but I don't like the idea of adding a bunch of endpoints for every use case when we can have a generic reusable solution.

[willrlin]

@anders-schneider @avaleske @abbiefarr FYI as it relates to data structures and back end.

[pbn4]

The idea looks OK to me. Possible problems:

• some getters we have defined in DTO assume certain fields are joined,
• tests against SQL injections shoud be added.

Regarding the parser: Entity and it's relations represent a tree so we could create an input model from id,name,units.id,units.unitType.id,units.unitType.name which could look like this:

{
   "id":0,
   "name":0,
   "units":{
      "id":0,
      "unitType":{
         "id":0,
         "name":0
      }
   }
}

consider 0 a leaf node. We could build a query by traversing it recursively, so for each level we do not need to keep joins memoization array. One possible problem with this query string approach in general is that browsers only allow 2048 characters in a query string and listing with subfields contains a lot of properties to join and select. It could e.g. be solved by a default logic selecting all properties of a relation if no explicit columns have been selected in the query tree e.g. {"events": 0} would select select all columns of events relation by default, but {"events": {"id"}} would result in only "id" being selected. If it comes to relations, they probably shouldn't be joined by default if not explicitly specified (same as in typeorm querybuilder).

[seanmalbert]

The tree you suggest makes sense to me, and if we did want to have the logic you suggest where we select all properties of a relationship if none are specified (assuming the relationship was specified), that would work better.

In regards to the query string limit, we could update these to POST, so we don't run into that. There are other benefits to doing this like having easier to read/write queries from the client by passing parameters in the JSON body. Imagine something like:

{
  "fields": "id, name, units",
  "filter": {
    "name": "foo",
    "status:" : {  "$neq": "pending" }
  },
  "options": {
    limit: 25,
    page: 1
  }
}

[avaleske]

This contrasts a bit with the approach we took for our filters and pagination work, which I'm preparing to contribute upstream to you in #1578 (There's a few frontend fixes I need to make before mailing this PR, which I'll do tomorrow.)

As I noted in the PR description, rather than exposing arbitrary fields, we opted to keep the backend opinionated about which fields and filters it exposes to the frontend. This keeps the frontend from having to know anything about the database schema, which seemed preferable to us.

The code that actually applies the filters is still generic and could be used for other entities, but only the fields listed in ListingFilterKeys and filterTypeToFieldMap are exposed to the frontend. This way, if the data model changes we just update the map; the frontend doesn't need to know.

I really like the idea of specifying the fields the frontend actually cares about, instead of requesting everything. What if, instead of the frontend requesting exactly the fields it needs (which would require the frontend knowing about the database schema, which is what we were trying to avoid with our filters strategy) the backend defines several "lenses" or "views" on the entity and the frontend can request the view it wants?

Some rough details:

• A "view" would just be a named list of the fields that the frontend cares about, like simpleListings, fullListingsWithUnits, or fullListingsWithUnitsAndAmi. They could probably inherit from or extend each other as well.
• The frontend would request a view via a queryparam, like \listings?page=2&limit=10&view=simpleListings
• The backend's getQueryBuilder() in the listings service would take the view param and adjust the selects and joins accordingly, by checking which fields the view includes.

This is more resilient to the database schema changing, and won't require updating every data call from the frontend, just the view definitions.

(@willrlin and @anders-schneider for visibility)

[avaleske]

Glad it was helpful! We'll definitely need a solution to reduce the amount of data sent over the wire, since we're not currently pre-rendering the /listings view on the Detroit fork (Pre-rendering every permutation of filters and pages seemed excessive, and I haven't figured out how to only pre-render the first /listings page, which we have a ticket to fix: CityOfDetroit#149)

I've been interested in GraphQL but I've never tried it. I saw #77 and I'm interested to see what you end up deciding if you revisit it.

[seanmalbert]

@avaleske ,
I'm not sure on your current implementation, but you could have a statically generated /listings page and if a user filters or pages you could route them to a different, non statically generated page.

For example, we have a preview page at /sites/public/pages/preview/listings/[id].tsx (url path of /preivew/listings/[id]), which uses NextJS getServerSideProps to get the id parameter and build the page at runtime. Of course, you don't have to use that function and do a traditional fetch data hook, but this may be an option for you.

You may also want to look into using an older API getInitialProps, which supports the use of query strings where getStaticProps/Paths does not.

[seanmalbert]

What may also be of interest, is that we created #1619 , with the intent to reduce the amount of calls we have to the backend during build/deployment.

[seanmalbert]

@avaleske, I created #1621 to handle creating a simple view. Do you mind taking a look and see if something like this would also meet Detroit's needs? It does reference the current way we're dealing with unit summaries and I understand there are changes coming around that, but I'd like to get this going for our needs soon.

[avaleske]

I added my thoughts to #1621 but tldr: that's basically what I was picturing!

And re filters and static generation: I like your idea of having a statically generated page that just routes to a different path for standard client-side react filtered pages. That also helps keep our frontend filter code separate so we don't diverge too far from you. I'll have to look at getServerSideProps and getInitialProps again too, thanks!