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

add a query parameter to disable redis caching #224

Closed
newgene opened this issue Jul 20, 2021 · 8 comments · Fixed by #225 or #350
Closed

add a query parameter to disable redis caching #224

newgene opened this issue Jul 20, 2021 · 8 comments · Fixed by #225 or #350
Assignees
Labels
bug Something isn't working

Comments

@newgene
Copy link
Member

newgene commented Jul 20, 2021

By default, Redis caching is enabled when REDIS_HOST and REDIS_PORT env variables are provided. Sometime, for debugging purpose, just to rule out the issue with the caching, we want to query BTE without caching. An extra query parameter like caching=false can be added to temporarily disable the caching.

Relevant code is here:

https://github.com/biothings/bte_trapi_query_graph_handler/blob/main/src/cache_handler.js

@newgene newgene added the enhancement New feature or request label Jul 20, 2021
@ericz1803
Copy link
Contributor

Just to clarify, you want an option for the user to specify a caching param in the request?

@newgene
Copy link
Member Author

newgene commented Jul 22, 2021

This is what I have in mind:

Dy default, the query is submitted like this:

POST /v1/query (with the body as the query graph)

With an optional query parameter, we can make the same query like this:

POST /v1/query?caching=false (body is the same query graph)

to disable the default caching behavior.

@ericz1803 ericz1803 linked a pull request Jul 22, 2021 that will close this issue
newgene added a commit that referenced this issue Jul 27, 2021
Feature: Add caching query param (Fix issue #224)
@andrewsu andrewsu added bug Something isn't working and removed enhancement New feature or request labels Nov 5, 2021
@andrewsu
Copy link
Member

andrewsu commented Nov 5, 2021

When I post a query to https://api.bte.ncats.io/v1/query?caching=false, I get the following error:

{
    "error": "Your input query graph is invalid",
    "more_info": [
        "query should NOT have additional properties 'caching'"
    ]
}

I've tested a handful of queries with the same behavior, so it does not appear to be query-dependent. I can also reproduce this on my local instance when sending a query to http://localhost:3000/v1/query/?caching=false.

@colleenXu
Copy link
Collaborator

colleenXu commented Nov 5, 2021

@andrewsu should I also modify BTE's smartapi registration to include the caching parameter (for query endpoints)? https://github.com/NCATS-Tangerine/translator-api-registry/blob/32737525ff94ad8f5223d8a4f77031d6cca4d91e/biothings_explorer/smartapi.yaml#L100

I have a bit of an idea of how to do that, but not fully sure about the openapi/smartapi spec...

[EDIT] looks like what eric edited was a local copy of the smartapi registration yaml...

@andrewsu
Copy link
Member

andrewsu commented Nov 5, 2021

huh, interesting question. Clearly the local version is necessary and used in this validation step: https://github.com/biothings/BioThings_Explorer_TRAPI/blob/main/src/middlewares/validate.js#L18. But the local version has also diverged from the smartAPI version here https://github.com/NCATS-Tangerine/translator-api-registry/blob/master/biothings_explorer/smartapi.yaml. I don't particularly like to have two versions hanging around, but we also don't want to validate every query against a file in another repo.

In the interest of expediency, I'm fine merging this PR just to fix the issue. But probably we should discuss if we want to change how we handle this...

@colleenXu
Copy link
Collaborator

@andrewsu my understanding is that the local version is meant to match the smartapi version...

@newgene
Copy link
Member Author

newgene commented Nov 5, 2021

@colleenXu you may try the latest BTE metadata from SmartAPI locally to see if any further changes are needed.

And I agree that this file should be sync'ed with the smartapi version.

@colleenXu
Copy link
Collaborator

colleenXu commented Nov 6, 2021

@newgene @andrewsu to clarify: what endpoints have the caching parameter?

Currently I've added updates to the PR for the local file #350 and the smartapi registration yaml https://github.com/NCATS-Tangerine/translator-api-registry/blob/master/biothings_explorer/smartapi.yaml, so they are identical.

Both files have the caching parameter specified on the /query endpoints only (v1/query, v1/smartapi/{}/query, and v1/team/{}/query)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants