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

Implementation of embedded client #2297

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

magnetised
Copy link
Contributor

@magnetised magnetised commented Feb 4, 2025

which talks to electric over the internal api, not http

  • also stop parsing offsets within the client. just pass around the string version

@magnetised magnetised force-pushed the 2256-add-embedded-client-implementation branch 2 times, most recently from 172076c to def5564 Compare February 4, 2025 16:12
@magnetised magnetised marked this pull request as ready for review February 4, 2025 16:13
@magnetised magnetised force-pushed the 2256-add-embedded-client-implementation branch 2 times, most recently from 86fa1a4 to d3fd28a Compare February 5, 2025 09:42
@magnetised magnetised force-pushed the 2256-allow-for-a-pure-elixir-api branch from ea6edeb to c0e4d05 Compare February 5, 2025 13:53
@magnetised magnetised force-pushed the 2256-add-embedded-client-implementation branch 4 times, most recently from d302051 to 71f6e0e Compare February 6, 2025 09:07
Base automatically changed from 2256-allow-for-a-pure-elixir-api to main February 6, 2025 09:19
@magnetised magnetised force-pushed the 2256-add-embedded-client-implementation branch from 71f6e0e to 78ce6d0 Compare February 6, 2025 09:21
which talks to electric over the internal api, not http
@magnetised magnetised force-pushed the 2256-add-embedded-client-implementation branch from 78ce6d0 to db7c4a3 Compare February 6, 2025 09:41
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice !

quote(
do: %ControlMessage{
control: :up_to_date,
offset: offset(unquote(tx), unquote(op))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still include offsets in the control messages?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never did on the wire, actually

@@ -47,6 +47,10 @@ defmodule Electric.Plug.Utils do
end)
end

def parse_columns_param([col | _] = columns) when is_binary(col) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: techincally just ensuring the first col is binary (and not if they are valid postgres identifiers) - you could potentially have the string splitting in L34 as a separate step and then recursively call into itself to do the rest of the parsing?

def type, do: {:array, :string}

def cast([_ | _] = columns) do
{:ok, Enum.map(columns, &to_string/1)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: potentially worth passing through the parser again to check that they are valid pg identifiers etc? see above comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants