EctoSource: Use schema prefix, if there is one

[jfornoff]

If you have a prefixed schema, Honeydew will just use the table name without the prefix that is specified as @schema_prefix.

This PR makes sure that the prefix is applied correctly.

[koudelka]

hey there, thanks for the PR!

instead of concatenating the prefix to the table, can you pass the prefix to the various database-specific SQL modules, and have them do it? i don't think it's entirely safe to make the assumption that every database is going to handle prefixes the same way.

also, could you include some tests?

thanks!

[jfornoff]

Hi @koudelka,
yeah that is a good point for sure, we are not sure whether all the engines do prefixing identically.
One way that we could explore is letting Ecto do the heavy lifting, it does prefixing automatically on its queries.

AFAICT Honeydew.EctoSource.SQL.<Postgres / Cockroach>.reserve bites us, since it just executes plain SQL and the prefixing is not applied there.
Maybe it'd be worth checking if that could be done in terms of Ecto to make sure the library doesn't care too much about DB concerns, what do you think?

Best,
Jan

[jfornoff]

Okay I've spent some time on it, and porting to Ecto doesn't work great with version 2, especially since we are trying to update a dynamic column (i.e., the lock column) with a fragment when reserving, that became way easier in Ecto 3.0.5 apart from that, I got the query for reserving lined up.

At the moment, this approach seems correct but hardly feasible.

[jfornoff]

From what I can tell, Postgres and Cockroach have the same table name semantics, so it's pretty much the same for both.

[koudelka]

hey, looks great, would you mind adding some tests though?

you also mentioned something about porting to Ecto? did you mean porting the queries themselves to Ecto's query DSL, rather than using string fragments?

[jfornoff]

Exactly, I tried that on my fork and it worked somewhat okay, but with Ecto 3 it'd be easier for sure. Hope that I get to adding tests later!

[koudelka]

sorry do you mean that you're not going to add tests now? :(

[jfornoff]

No I will absolutely, no worries :-)

[jfornoff]

@koudelka sorry for the delay, i added integration test coverage for the prefixed case.
CI seems to be green, the failing test appears to be a transient failure.

Let me know if there's anything else that we should add :-)

[koudelka]

instead of an explicit conditional here, what do you think of doing the matching in the function head instead? like

defp add_prefixed_tables_env(env, true) do
  env ++ [{"prefixed_tables", "true"}]
end

defp add_prefixed_tables_env(env, false), do: env

[jfornoff]

SGTM! If you want you can just push it if you're on it, otherwise I can make the change tomorrow as well 👍

[koudelka]

published v1.2.7 on hex, thanks again dude. :)

[jfornoff]

Nice, thanks!

…

On Wed, Jan 9, 2019, 00:16 Michael Shapiro ***@***.*** wrote: published v1.2.7 on hex, thanks again dude. :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#67 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQjiiHboEFrrYQvkBTX1eyI2zwfKVkbks5vBSangaJpZM4ZLxP3> .