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

Update backend to match upstream changes #143

Closed
Tracked by #63
avaleske opened this issue Jul 7, 2021 · 4 comments · Fixed by #180
Closed
Tracked by #63

Update backend to match upstream changes #143

avaleske opened this issue Jul 7, 2021 · 4 comments · Fixed by #180
Assignees
Labels
epic: filtering Tickets related to Filtering size: 3 About a day of work
Milestone

Comments

@avaleske
Copy link

avaleske commented Jul 7, 2021

Upstream added a basic filtering capability that will conflict with ours.

They added it in bloom-housing#1359

It's probably a day's work to update our work to match.

@avaleske avaleske mentioned this issue Jul 7, 2021
35 tasks
@avaleske avaleske added epic: filtering Tickets related to Filtering size: 3 About a day of work M7 labels Jul 7, 2021
@avaleske avaleske self-assigned this Jul 7, 2021
@avaleske avaleske added this to the M7 milestone Jul 7, 2021
@avaleske
Copy link
Author

avaleske commented Jul 9, 2021

Ok I figured out how Exygy's filter code works! It's clever, which makes it extensible, but also confusing.

Summary

With a URL like /listings?filter[$comparison]==&filter[depositMin]=500&filter[$comparison]=<>&filter[name]=Coliseum
we end up with a filter param in the listings controller that looks like this:

{
  $comparison: [
    '=', 
    '<>'
  ],
  depositMin: '500',
  name: 'Coliseum'
}

The addFilter code that's called from the listings service then iterates through these fields to add andWhere clauses to the query builder, using each successive comparison operator.

The code that adds that clause looks like

qb[operator](`${schema}.${key} ${comparisons[comparisonCount]} :${key}`, {
  [key]: val,
})

So we'd end up with
qb['andWhere']('listings.depositMin = :depositMin', { depositMin: 500 })
and
qb['andWhere']('listings.name <> :name', { name: Coliseum })

Other notes

  • The <> operator is the same as !=
  • To add multiple filters with the same key, a URL might look like /listings?filter[$comparison]==&filter[depositMin]=500&filter[$comparison]=<>&filter[depositMin]=400&filter[$comparison]=<>&filter[name]=Coliseum, which comes through as
{$comparison: ['=', '<>', '<>'], depositMin: ['500', '400'], name: 'Coliseum'}
  • There's a bug: You need to group a comparison and the filter together, and you also need to group any filters of the same type together. If you have depositMin, then name, then depositMin again, the framework will group the depositMins together, but won't re-order the comparisons, so you'll be using the wrong comparison in the query.
  • I think this enum is trying to restrict the comparison operators that re allowed, but it doesn't seem to work?
  • This only adds andWhere clauses, so it wouldn't support filtering to multiple neighborhoods. It'd take some additional logic to do that.

Next steps

  • Write some documentation for this
  • I want to restrict the fields that can be used as filter params. Right now, a user could filter on any field name in the database
  • I don't like how much of the internal database schema this exposes to the outside; it's a really leaky abstraction. So I'm going to have an internal map from filter name to field name, so the implementation details of the database don't need to be known by the frontend.

@anders-schneider
Copy link

That summary is so helpful!! One question:

Right now, a user could filter on any field name in the database

On first glance, it looks like the purpose of ListingFilterParams is to specify which fields could be filtered on, but it sounds like that's not the case? But maybe it's meant to be the case? I feel like this question concerns how the url string gets parsed into a ListingFilterParams instance, which maybe is dictated by the ApiQuery decorator that I wish I understood better...

@willrlin
Copy link

willrlin commented Jul 9, 2021

Do you think it's worth having a discussion with Exygy about simplifying this or proposing our own alternative?

@avaleske
Copy link
Author

avaleske commented Jul 9, 2021

I think so, yeah. Or at least making sure they're ok with my idea of mapping from filter name to database field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic: filtering Tickets related to Filtering size: 3 About a day of work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants