adsabs / adsws

ADS web services
Other
2 stars 14 forks source link

Added solr route cookie for solr-service requests #143

Closed marblestation closed 6 years ago

marblestation commented 6 years ago

When a request is directed to solr-service, the user token is extracted from the request and its solr route (which link to a specific solr instance) is retreived from redis (if it exists) before passing the request to solr-service.

Solr-service response is also inspected for "Set-Cookie" instructions with the solr route, in case it is present the solr route is stored in redis using the user's token as key.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.4%) to 82.655% when pulling d60a35dc0d2a7e5e53be32bc54bc0c849eb5da23 on marblestation:set_solr_route into 2b3655bfd57452d6f756663c751e14b9f9bc5237 on adsabs:master.

romanchyla commented 6 years ago

@marblestation i can't agree with the latest change - the /search is IMO worse than the module path (and can be changed via config)

but the most important thing was the remote context; that seems to be not addressed at all; unless i'm not seeing all commits: https://github.com/marblestation/adsws/blob/491f9f60b7c0238c1d885750b730cdfdbefb21f2/adsws/api/discoverer/utils.py#L95

marblestation commented 6 years ago

The remote context was addressed here: https://github.com/marblestation/adsws/blob/491f9f60b7c0238c1d885750b730cdfdbefb21f2/adsws/api/discoverer/utils.py#L178

And actually that's what motivated the change to deploy_path == '/search', for remote services we do not have package names. I thought it would be good to have both (local and remote) to use the same criteria, and I thought deploy_path was a good way to go because if in the future we move solr-service from a local to a remote service, we may keep the same URL. What is your preferred solution? Should I change back the condition for the local service? I don't really have a strong opinion on this.

romanchyla commented 6 years ago

ok, sorry, here is the explanation - i was taking a mental shortcut

first, the local/remote handling look identical - and I didn't notice that the remote was there, my bad. secondly, and that became clear to me only now: I was thinking "the mechanism should be generic".

If you notice, we are hardcoding a tight coupling right into the heart of adsws (which escaped me at first, but now I see it clearly) -- adsws shouldn't know/care if it is running /search or treat some module specially

So I'd say instead of

if deploy_path == '/search':
     view = solr_view......

you do:

if deploy_path in current_app.config.get('AFFINITY_ENHANCED_ENDPOINTS', []):
    view = affinitiy_view(AFFINITY_PREFIX=deploy_path,....)(view)
marblestation commented 6 years ago

I agree with your suggestion, I tried to implement something in those lines. What do you think now?