opencivicdata / imago

django app powering Open Civic Data API
Other
12 stars 12 forks source link

Get sort query parameter to work again #75

Closed evz closed 8 years ago

evz commented 8 years ago

As mentioned in #51, sorting the API responses doesn't work. When one digs into why it was failing, it had to do with the way that Django was attempting to construct the DISTINCT ON clause for PostgreSQL. Since PostgreSQL requires that the ORDER BY portion of the query have fields in the same order as the fields in the DISTINCT ON portion of the query, and Imago just appeared to be constructing the query one step at a time, Django didn't really know what to do in that situation.

This PR just removes the DISTINCT ON thing since there really shouldn't be a situation where there is more than one object with the same ID. Does this seem sane?

jpmckinney commented 8 years ago

Can you split out the following into separate PRs:

jpmckinney commented 8 years ago

Hmm, nevermind - (some of) those commits already exist in master, but for some reason show up in the PR.

jpmckinney commented 8 years ago

@paultag added that line in https://github.com/opencivicdata/imago/commit/d977bba960c9596da2dafecf596f6d3338b08d92 so I assume there's a reason for it.

evz commented 8 years ago

@jpmckinney Yeah, I was kinda scratching my head about that, too. As far as I could tell, I was keeping my fork synced with opencivicdata.

evz commented 8 years ago

@jpmckinney I figure it's there for a reason. It's just causing Django to not know what to do when you're wanting to sort. I was hoping we could get to the bottom of it here.

jpmckinney commented 8 years ago

I mentioned paultag, since he would know what's going on - but I figure the preceding lines must sometimes return duplicates.

fgregg commented 8 years ago

ping @paultag

fgregg commented 8 years ago

@jpmckinney, here's the word from paultag

11:45 <+paultag> I'm afraid I can't gain context at the moment, but it's been 
                 on my queue
11:45 <+paultag> if my actions looked irrational, ignore them and revert
jpmckinney commented 8 years ago

Actually, can you rebase so that all the old commits don't show up here? I don't know what I'm commenting on.

fgregg commented 8 years ago

Replaced by #77