jaeyson / ex_typesense

Typesense client for Elixir with support for importing your Ecto schemas.
https://hexdocs.pm/ex_typesense
MIT License
30 stars 11 forks source link

Configuring a connection string dynamically #17

Closed javiertoledo closed 5 months ago

javiertoledo commented 6 months ago

Hi there, first of all, thanks for this library, it's being quite useful!

I'm working on a small internal tool that basically keeps track of our different database instances and performs health checks across our company, and I wonder if there is a way to configure the connection parameters dynamically from a connections database in which I store the URL and encrypted API keys for each connection. I saw in the code that you're loading the connection parameters from the Application environment in the HttpClient module, so I'm guessing that the answer is no, but I'm not an expert in Elixir, and I wanted to ask anyway, just in case there's some elegant way to override the default behavior of the HttpClient that I don't know.

If there's no other way, I guess I could always fork this repository and add the feature myself. Would you be interested in incorporating this feature into this codebase?

Thanks!

jaeyson commented 6 months ago

Hi @javiertoledo 👋🏾, Thank you for stopping by.

I can entertain such request, but allow me to ask few questions:

I wonder if there is a way to configure the connection parameters dynamically from a connections database in which I store the URL and encrypted API keys for each connection.

Aside from loading dynamic api keys, may I know further details? e.g. you need request headers to be added dynamically?

I saw in the code that you're loading the connection parameters from the Application environment in the HttpClient module

To configure, one can add the connection string via runtime (config/runtime):

# add this to your Phoenix app in config/runtime.exs if it's a local instance
config :ex_typesense,
  api_key: "xyz",
  host: "localhost",
  port: 8108,
  scheme: "http"

 # or maybe you're using a cloud instance provide by Typesense
config :ex_typesense,
  api_key: "credential",
  host: "111222333aaabbbcc-9.x9.typesense.net" # Nodes
  port: 443,
  scheme: "https"

Or you mean change this from an app config to a function?

instead of:

def get_host, do: Application.get_env(:ex_typesense, :host)
def get_port, do: Application.get_env(:ex_typesense, :port)
def get_scheme, do: Application.get_env(:ex_typesense, :scheme)
def api_key, do: Application.get_env(:ex_typesense, :api_key)

we can:

# defaults are provided to use for localhost, except setting api keys
def set_host(host \\ "localhost"), do: host
def set_port(port \\ 8108), do: port
def set_scheme(scheme \\ "http"), do: scheme

def set_api_key(api_key) do
  api_key ||
    raise """
    Typesense API key is missing. Set your key if local, or for more
    info: https://typesense.org/docs/latest/api/api-keys
    """
end

so you can load it dynamically, correct?

Once I have all the details you provide for changes, I'll pull request (with your credit and direction).

javiertoledo commented 6 months ago

Yes, the idea is that I can dynamically set the connection parameters, but I would instead look for a stateless solution because I'm not sure about the effects that setting the connection details before every call may have in a multi-threaded environment.

For instance, imagine I have a connections table and a Connection Ecto Schema, I'd like to load the connection parameters from my PostgreSQL table and inject them directly as the first parameter of the ExTypesense:

query_results = Connections.get_connection!(connection_id)
|> ExTypesense.search(collection_name, query)

Looking at the ex_typesense codebase, I think this would imply a significant refactor, and probabñy introduce a breaking change. It would probably be easier to add an optional last argument on all functions that defaults to the connection details in the Application context so I can do this instead:

connection_details = Connections.get_connection!(connection_id)
query_results = ExTypesense.search(collection_name, query, connection_details) # Omitting the `conection_details` parameter here would just fall back to the values set in the Application context.

This way would avoid introducing a breaking change and I could still override the connection, but of course, this solution would be slightly less delightful to use as I could no longer pipe the connection details into ExTypesense methods 😅

I don't know, maybe there's some Elixir black magic that can be done here to solve this more elegantly, I'm looking forward to learning more about this interesting programming language :-D

jaeyson commented 6 months ago

Ah, makes sense now! I think we can add that as another function (using cons |> aka pipe operator) or refactor to accommodate connection strings. I'll ping you up when I start a PR. Thanks!

javiertoledo commented 6 months ago

Thank you!

jaeyson commented 6 months ago

@javiertoledo kindly check #18. It's still WIP and the functions that is complete are in collections and cluster module.

javiertoledo commented 5 months ago

I did a quick review, and I think it's looking really good. I've also learned a few Elixir tricks I didn't know :-) I'll try the changes in my project soon and let you know what I find. Thanks a lot for working on this!

jaeyson commented 5 months ago

@javiertoledo Anything else that I missed? should I merge this to main?

javiertoledo commented 5 months ago

Hi @jaeyson, I finally had the chance to try your changes, and it works flawlessly, exactly as I imagined. Very elegant solution, great work! Thanks a lot for adding this feature, it's very helpful to us :-)