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

API Behaviour: authentication and switching between local db and API #280

Closed
2 of 3 tasks
PietrH opened this issue Jun 6, 2023 · 8 comments
Closed
2 of 3 tasks
Assignees
Labels

Comments

@PietrH
Copy link
Member

PietrH commented Jun 6, 2023

This is a technical issue to capture a discussion regarding the future migration of ETN to using an OpenCPU API.

Function Naming

  • Functions will retain the same names, and no exported duplicate functions will be made.

  • The functions will allow users to switch between using the API, or a local database connection (as they do now on the RStudio Server instance).

Authentication and switching between the API and a local database connection

flowchart TD

main(("list_animal_ids(
    other_arguments,
    connection = NULL,
    api = TRUE
    )")) --api=true--> api_helper(("list_animal_ids_api(
    other_arguments
    )"))
main --api=false--> sql_helper(("list_animal_ids_sql(
other_arguments
)"))
main --"lifecycle::is_present(connection)"-->warning[["warning: connection is depreciated,
please set API=FALSE to use local db"]]
warning --> sql_helper
api_helper --> get_credentials1["credentials = get_credentials()"]
api_helper --> other_arguments1["other_arguments"]
get_credentials1 --> etnservice[/"ETNSERVICE"/]
other_arguments1 --> etnservice
sql_helper --> get_credentials2["credentials = get_credentials()"]
get_credentials2 --> connection
sql_helper --> other_arguments2["other_arguments"]
connection --> code>"existing code"]
other_arguments2 --> code
Loading
  • Authentication via the API uses an internal helper get_credentials() that gets the username/password from the environment (or later on, asks the user to fill them in, warns for missing/empty pwd, uses keyring similar to move2)

  • Users will be able to pass a connection argument as they do now, and if they do so, ETN will try to use a local database connection. Old scripts will continue to work on the RStudio server. However, the contents of the connection argument are unused, as the SQL helper will use get_credentials() to build a connection argument for itself internally.

  • If a user wants to run an old script locally, using the API, all they need to do is remove the connection argument.

  • If a user wants to use the local db, they set API = FALSE

  • The API and local db helpers both use a credentials argument populated with get_credentials().

  • main can be merged into this script that contains both helpers, the changes to main need to be merged manually into the sql helper

  • This allows for switching between api and local db use, allows for the reuse of existing scripts when run on the RStudio Server, allows adaptation of existing scripts with minimal effort (as no authentication argument needs to be added for functions to work)

  • Currently all functions require authentication, but in the future we can consider disabling authentication for functions that report data without an embargo: this is a database level change

Idea's

@PietrH PietrH self-assigned this Jun 6, 2023
@PietrH PietrH added this to the v3.0.0 milestone Jun 7, 2023
@PietrH PietrH mentioned this issue Jun 7, 2023
55 tasks
@PietrH PietrH mentioned this issue Jun 7, 2023
34 tasks
@PietrH
Copy link
Member Author

PietrH commented Jun 7, 2023

Should the tests for the API only, or once for the API route, and once for the SQL route?

For integration into github actions, only the API route makes sense.

@PietrH
Copy link
Member Author

PietrH commented Jun 8, 2023

get_credentials() now prompts the user when no values are stored in sys.env: d1a2b06

@PietrH
Copy link
Member Author

PietrH commented Jun 8, 2023

I'm now using a generic api helper instead of a api helper per function: 8d9a001

@PietrH
Copy link
Member Author

PietrH commented Jun 8, 2023

The code used for the main functions is very repetitive, and suitable for wrapping in a helper. This would make changes to this code easier in the future and would keep it DRY.

However, it currently is immediately apparent what the main function is doing (some parsing, and passing arguments on to two helpers, one general, and one specific). Using a helper to do this, would detract from the functions being self explaining.

@PietrH
Copy link
Member Author

PietrH commented Jun 9, 2023

Instead of having a helper function with almost the same code as etnservice, wouldn't it be cool if the helper could instead just get the code from etnservice directly and evaluate it in place for it's sql functions?

That way we only have to maintain the functions in one place.

@peterdesmet
Copy link
Member

Right, we currently need to maintain it in two places ... We could load etnservice as a dependency, but then we also bring in its dependencies.

@PietrH
Copy link
Member Author

PietrH commented Jun 12, 2023

etnservice will, at least for the moment have the same dependencies as etn as they are both running the same sql functions. etn also adds some packages for handling the communication with the api.

I think loading etnservice as a dependency is the way forward, but for now I'll keep going ahead with a hard coded _sql helper. And migrate to using etnservice when we are bit further ahead.

@PietrH
Copy link
Member Author

PietrH commented Oct 22, 2024

  • etn is now dependent on etnservice, both directly for local database queries, as indirectly for API requests to OpenCPU.
  • There is a one on one relation between the functions between the packages, but every function still has it's own file.
  • Client side processing is possible (and happens in the case of download_acoustic_data(), which is completely client side usage of other functions).
  • etnservice is no longer dependent on etn, but shares the same test suite (not in sync at the moment).
  • All functions require authentication, but credential caching is used by default.

@PietrH PietrH closed this as completed Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants