search

fecgov / openFEC-web-app

DEPRECATED See https://github.com/18F/fec-cms for fec.gov's code
Other
43 stars 31 forks source link

Legal search results URL is prefixed with `/data` #1193

Closed adborden closed 6 years ago

adborden commented 8 years ago
  1. Open https://fec-proxy.18f.gov/legal-resources/
  2. Enter a search term, hit enter

Actual URL is prefixed with /data https://fec-proxy.18f.gov/data/legal/search/?search_type=all&search=computer

Expected URL is not prefixed https://fec-proxy.18f.gov/legal/search/?search_type=all&search=computer

I was hoping to solve this with the newly supported cf map-route --path command, but that would require resolving https://github.com/18F/openFEC/issues/1621, which sounds like won't be supported for a while longer.

The other thought is to use the existing proxy, which would require creating a new /legal/ route, and serving search under /legal/legal/search from the web app. The proxy strips out the mapping prefix, so user would see it as /legal/search.

Thoughts @LindsayYoung @jmcarp

jmcarp commented 8 years ago

At the risk of restating the problem: all routes in this app (openFEC-web-app) are prefixed with /data under the proxy configuration--so it's expected (from my perspective, at least) that the /legal path would be prefixed too. I'm inferring from this issue that's not the behavior you want, but I'm not sure why.

Would using cf map-route --path help here? AFAIK, that option maps an entire application to a sub-path. And since all the routes in this repo are deployed as the same application, I'm not sure what that buys us. Adding a rule to the proxy config should work, except that I'm not sure whether links between legal and data routes will be correct--if I'm remembering how flask handles the X-Script-Name header correctly, links from legal to data will inappropriately prefix with /legal instead of /data.

There's also a totally different approach, which is to run everything in the same python process and mount sub-applications at arbitrary paths using e.g. werkzeug's DispatcherMiddleware: https://github.com/pallets/werkzeug/blob/5a2bf35441006d832ab1ed5a31963cbc366c99ac/werkzeug/wsgi.py#L630-L659. Then you could implement data and legal resources as separate apps without relying on cloud foundry or a separate proxy.

adborden commented 8 years ago

At the risk of restating the problem: all routes in this app (openFEC-web-app) are prefixed with /data under the proxy configuration--so it's expected (from my perspective, at least) that the /legal path would be prefixed too. I'm inferring from this issue that's not the behavior you want, but I'm not sure why.

You're totally right, but that's not the behavior I want. The goal of this issue is to have /legal/search not prefixed by anything (or something not /data if we can't get around it). Some background: we put the legal resources search in openFEC-web-app to avoid creating yet another app and from our perspective, openFEC-web-app didn't look specific to campaign finance data. I would describe it as home to any non-cms content including campaign finance data or legal resources search. To prefix the entire app under /data implies it's all campaign finance data related which I see as a bug. Hope that makes sense?

Would using cf map-route --path help here? AFAIK, that option maps an entire application to a sub-path. And since all the routes in this repo are deployed as the same application, I'm not sure what that buys us. Adding a rule to the proxy config should work, except that I'm not sure whether links between legal and data routes will be correct--if I'm remembering how flask handles the X-Script-Name header correctly, links from legal to data will inappropriately prefix with /legal instead of /data.

I'm not familiar with the X-Script-Name. I saw it in the fec-proxy but didn't see how it was being used in the Flask app. I assumed the web-app was just routing based on path, so maybe I'm missing some information?

The routing stuff is all superficial and I'm not arguing any technical reason for it besides making sure the cms app doesn't serve routes intended for /data or /legal.

I want to point out the main difference I found with fec-proxy and cf map-route --path is that if you had a route /legal/stuff, fec-proxy strips out the matched prefix, so your app sees /stuff but with the cf route, your app sees the full path /legal/stuff. So my suggestion to use cf map-route --path implies the additional change of mounting the data routes under /data and the legal routes under /legal within openFEC-web-app. Sorry if that wasn't clear.

There's also a totally different approach, which is to run everything in the same python process and mount sub-applications at arbitrary paths using e.g. werkzeug's DispatcherMiddleware: https://github.com/pallets/werkzeug/blob/5a2bf35441006d832ab1ed5a31963cbc366c99ac/werkzeug/wsgi.py#L630-L659. Then you could implement data and legal resources as separate apps without relying on cloud foundry or a separate proxy.

How would the cms app know not to serve routes from this app? Wouldn't you still need some kind of prefix for fec-proxy to route around?

adborden commented 8 years ago

Just following up here, @jmcarp and I spoke offline to get on the same page. We decided to see if we could patch the cf cli as per https://github.com/cloudfoundry/cli/pull/747 and concluded the the change we need is little more involved. Specifically, it looks like we need to support multiple routes in the manifest, either (/data and /legal) or (/ and /legal).

Instead, we'll investigate making this fix via fec-proxy.