Triplex.all should not return prefixes

[kelvinst]

It should take config().tenant_prefix out. It is a breaking change though, as some people might be relying on the fact that it returns the prefixes.

A Triplex.migrate_all would be nice addition too.

[abarr]

Hi,

I will have a look at this (As per email). I am happy for yuo to assign it to me if you like.

Andrew

[abarr]

Hi Kelvin,

I have been looking through the code. Without testing it looks like:

1. Remove :tenant_prefix from the Config module
2. Update Triplex.to_prefix(tenant, prefix \\ config().tenant_prefix) to remove the reference to the :tenant_prefix
3. Update the tests and Docs

Does this align with your thinking?

Regards,

Andrew

[abarr]

Hi,

I will also create a separate issue for the addition of Triplex.migrate_all

Andrew

[kelvinst]

Hi Kelvin,

I have been looking through the code. Without testing it looks like:

1. Remove :tenant_prefix from the Config module
2. Update Triplex.to_prefix(tenant, prefix \\ config().tenant_prefix) to remove the reference to the :tenant_prefix
3. Update the tests and Docs

Does this align with your thinking?

Regards,

Andrew

Not quite, the idea is to do something like String.replace(item, config().prefix, ""), so that Triplex.all returns the list of tenants without the actual prefix, so people can use Triplex.all to run migrations inside all tenants.

[kelvinst]

Today, Triplex.all actually return the schema names, so if the db has one tenant called "test" and the prefix is "tenant_", it will return ["tenant_test"]. The problem with that is that if someone does something like Triplex.all() |> Enum.each(&Triplex.migrate/1) it will not work, as it will iterate over the list to run Triplex.migrate which does apply the prefix again, so it will try to migrate a tenant "tenant_tenant_test", that does not exist.

[abarr]

Got it. Thanks

I will go back and look again.

Andrew

[abarr]

Hi Kelvin,

I am new to contributing to open source ... will you merge the pull request with master and then if you are happy bump the version number and create a branch? I just want to make sure I am making it as easy as possible for you.

Regards,

Andrew

[kelvinst]

will you merge the pull request with master and then if you are happy bump the version number and create a branch?

Yep, that's the plan

[abarr]

Hi Kelvin,

I have gone back through the code:

def to_prefix(tenant, prefix \\ config().tenant_prefix)

  def to_prefix(tenant, prefix) when is_map(tenant) do
    tenant
    |> tenant_field()
    |> to_prefix(prefix)
  end

  def to_prefix(tenant, nil), do: tenant
  def to_prefix(tenant, prefix), do: "#{prefix}#{tenant}"

I was wondering what the use case is for prepending the prefix field from the config?

Thanks,

Andrew

[kelvinst]

Basically, it is an option to add a prefix to all tenant schemas. That is good to avoid conflicts with other schemas on the db. Also useful to be allowed to use the id of a register as the name of a tenant, so that the prefix would make it a valid schema name.

[abarr]

Hi Kelvin,

I made the following changes based on our previous discussion:

def all(repo \\ config().repo) do
    sql =
      case repo.__adapter__ do
        Ecto.Adapters.MySQL ->
          "SELECT name FROM #{config().tenant_table}"

        Ecto.Adapters.Postgres ->
          """
          SELECT schema_name
          FROM information_schema.schemata
          """
      end

    %{rows: result} = SQL.query!(repo, sql, [])

    result
    |> List.flatten()
    |> Enum.filter(&(!reserved_tenant?(&1)))
    |> Enum.map(fn tenant_name ->
        case config().tenant_prefix do
          nil -> tenant_name
          _ -> String.replace_prefix(tenant_name, config().tenant_prefix, "")
        end
    end)

  end

It works fine for postgres but the mysql tests are failing because the tenant_table is adding the tenant name without the prefix.

def create_schema(tenant, repo \\ config().repo, func \\ nil) do
    if reserved_tenant?(tenant) do
      {:error, reserved_message(tenant)}
    else
      sql =
        case repo.__adapter__ do
          Ecto.Adapters.MySQL -> "CREATE DATABASE #{to_prefix(tenant)}"
          Ecto.Adapters.Postgres -> "CREATE SCHEMA \"#{to_prefix(tenant)}\""
        end

      case SQL.query(repo, sql, []) do
        {:ok, _} ->
          with {:ok, _} <- add_to_tenants_table(tenant, repo),
               {:ok, _} <- exec_func(func, tenant, repo) do
            {:ok, tenant}
          else
            {:error, reason} ->
              drop(tenant, repo)
              {:error, error_message(reason)}
          end

        {:error, reason} ->
          {:error, error_message(reason)}
      end
    end
  end

Before I go to further I wanted your guidance on whether this should be changed or if the code using the tenants_table should use to_prefix.

Thanks in advance.

Andrew

[abarr]

Hi Kelvin,

I have been thinking about my comment above.

It makes sense that the MySql functionality works in the same way as the Postgres code. When you get a list of tenants from the tenant table it should return the full schema name (i.e. prefix_tenant) and then it should be removed as per your preferred solution.

Are you happy with this approach?

Andrew

[kelvinst]

Yeah, postgres and mysql should work the same. Will review your work soon, thanks for the PR. Best, Kelvin Em 4 de nov. de 2020 00:00 -0300, Andrew Barr <notifications@github.com> escreveu:

…

Hi Kelvin, I have been thinking about my comment above. It makes sense that the MySql functionality works in the same way as the Postgres code. When you get a list of tenants from the tenant table it should return the full schema name (i.e. prefix_tenant) and then it should be removed as per your preferred solution. Are you happy with this approach? Andrew — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

[abarr]

Hi Kelvin,

Just checking in to make sure I have resolved the requested changes properly ... if not please let me know ... otherwise I expect you have been busy.

Andrew