feat: edgeql structure generation #168

deviprsd · 2023-09-26T12:03:57Z

https://discordapp.com/channels/841451783728529451/1152298319201173587

matching on structs is easier for LSP, so instead of returning the result as a map, the generator should return it as an instance of struct. and the user can use the comparison over the actual struct in the code
sometimes it can happen that elixir won't load atoms from typespecs. the current solution to this problem is the use of @Schema attribute just before the query function. but this looks weird and is actually a hack
the transformation of the result is done by the client and some kinda internal transform_result option. ideally the query module should do the mapping itself and not rely on the client for that

defmodule LiveBeats.EdgeDB.MediaLibrary.GetCurrentActiveSong do
 # other code
 
 defmodule Result do
   defmodule MP3 do 
    defstruct [
       :id,
       :filename,
       :filepath,
       :filesize,
       :url
     ]

     @type t() :: %__MODULE__{
       id: uuid(),
       filename: String.t(),
       filepath: String.t(),
       filesize: EdgeDB.ConfigMemory.t(),
       url: String.t()
     }
   end
   
   defmodule User do
     defstruct [
       :id,
     ]
     
     @type t() :: %__MODULE__{id: uuid()}
   end
   
   defstruct [
     :id,
     :artist,
     :title,
     :attribution,
     :date_recorded,
     :date_released,
     :paused_at,
     :played_at,
     :server_ip,
     :position,
     :inserted_at,
    :updated_at,
     :status,
     :duration,
     :mp3,
     :user,
   ]
   
   @type t() :: %__MODULE__{
     mp3: MP3.t(),
     user: User.t() | nil,
     artist: String.t(),
     title: String.t(),
     attribution: String.t() | nil,
     date_recorded: NaiveDateTime.t() | nil,
     date_released: NaiveDateTime.t() | nil,
     paused_at: DateTime.t() | nil,
     played_at: DateTime.t() | nil,
     server_ip: default__inet() | nil,
     id: uuid(),
     position: integer(),
     inserted_at: NaiveDateTime.t(),
     updated_at: NaiveDateTime.t(),
     status: default__song_status(),
     duration: duration()
   }
 end

 # rest of the code
end

Certain optimizations maybe part of a later improvement, for now the goal is get the basics generated and working.

… into edgeql-generation

nsidnev

@deviprsd

Nice work, I think this PR is ready for about 70-80 percent, which is great!

Please check my comments and fix them. If you have any questions, feel free to ask them here or in Discord.

I also suggest you run the tests so you can track if there's a issue in the code somewhere without waiting for me to look at PR update.

And to make it easier to debug the code you can run mix edgedb.generate, it will generate code for queries from test/support/codegen/edgeql and you can see if there are any bugs/errors in the generation. Also, when you are happy with the results, you should replace the .edgeql.ex.assert files queries directory with the .edgeql.ex.assert extension, since they are the main test sources for codegen that make sure that the behavior is correct.

nsidnev · 2023-10-06T07:05:08Z

CHANGELOG.md

+- support for generating Elixir modules from EdgeQL queries via `mix edgedb.generate`.
+- abitility to pass atoms as valid arguments for enums.
+
+### Changed
+- `jason` to be required library, but still configurable.
+- `EdgeDB.NamedTuple.to_map/2` to include indexes as keys into result map.


This is already in the Unreleased section, probably an artifact after a merge conflict

nsidnev · 2023-10-06T07:06:34Z

lib/edgedb/edgeql/generator.ex

@@ -61,7 +61,7 @@ defmodule EdgeDB.EdgeQL.Generator do
  EEx.function_from_file(:defp, :render_object_template, @object_template, [:assigns])
  EEx.function_from_file(:defp, :render_set_template, @set_template, [:assigns])

-  @spec generate(Keyword.t()) :: {:ok, %{Path.t() => Path.t()}} | {:error, term()}
+  @spec generate(Keyword.t()) :: {:ok, list(Path.t())} | {:error, term()}


generate/1 actually returns a map %{path to request file => path to generated module}

nsidnev · 2023-10-06T07:08:28Z

lib/edgedb/edgeql/generator.ex

  end

  defp codec_to_shape(%Codecs.Duration{}, _codec_storage) do
    timex? = Application.get_env(:edgedb, :timex_duration, true)

    typename = "duration()"

-    case Code.ensure_loaded?(Timex) do
+    case Code.loaded?(Timex) do


probably also wrong conflict resolution. this need to be Code.ensure_loaded?/1 because Code.loadled?/1 appeared only in Elixir >= 1.15

nsidnev · 2023-10-06T07:11:35Z

priv/codegen/templates/query.ex.eex

+          results = case results do
+            [] -> nil
+            _ -> Enum.map(results, fn result ->


this code will always execute the _ clause, because EdgeDB.query/4 returns {:ok, EdgeDB.Set.t()}, so we can just leave Enum.map/2 here

nsidnev · 2023-10-06T07:23:29Z

priv/codegen/templates/_object.eex

-        <% else %>
-            <%= name %>:
-        <% end %>
+@derive Jason.Encoder;


this should be rearranged to use the suggestions from credo. And it will be better for readability to add a blank line between the attribute, struct's definition and typespec's definition

nsidnev · 2023-10-06T07:43:49Z

priv/codegen/templates/_schema.eex

      <% name -> %>
-  :<%= name %>,
+        <%= name %>: result<%= Enum.map_join(@paths, &("[\"#{&1}\"]")) %>["<%= name %>"],


For the EdgeDB query from test/support/codegen/edgeql/standart/select_start_types_named_simple.edgeql this code generates the following mapping of the result:

%Result{ f: result["f"], e: result["e"], d: result["d"], c: result["c"], a: result["a"], b: %Result.B{b_b: result["b"]["b_b"], b_a: result["b"]["b_a"]} }

Nested calls won't be quite as performant (though not too slow), since in this case Elixir would have to first look at the result property and fetch the "b" object, and only then the "b_b" property. And then do the same for the `"b_a" property. But the main problem here is that it handles all types of links in the same way: required, optional and multi. This is a bug and will cause an error when running the code. This code should check the cardinality of a property and do something like that:

# if b is a required link %Result{ f: result["f"], e: result["e"], d: result["d"], c: result["c"], a: result["a"], b: with b <- result["b"] do %Result.B{b_b: b["b_b"], b_a: b["b_a"]} end } # if b is an optional link %Result{ f: result["f"], e: result["e"], d: result["d"], c: result["c"], a: result["a"], b: with b when not is_nil(b) <- result["b"] do %Result.B{b_b: b["b_b"], b_a: b["b_a"]} end } # and if b is a set %Result{ f: result["f"], e: result["e"], d: result["d"], c: result["c"], a: result["a"], b: Enum.map(result["b"], fn b -> %Result.B{b_b: b["b_b"], b_a: b["b_a"]} end), }

It's possible that this problem also affects normal properties, not just relationships, because I noticed for the query from test/support/codegen/edgeql/types/select_result.edgeql the following mapping

mp_named_tuple_x_int64_y_int64: %{ y: result["mp_named_tuple_x_int64_y_int64"]["y"], x: result["mp_named_tuple_x_int64_y_int64"]["x"] },

This should be wrapped in Enum.map/2 for cases where it is a mutli property or an array + this code doesn't account for indexes as fields for named tuples though they are in the typespec

nsidnev · 2023-10-06T07:50:26Z

priv/codegen/templates/_object.eex

    <% end %>
-}
+} <%= if @object[:is_optional] do %>
+| nil


This line is not necessary. Fields of a structure can be optional, but not the type itself, it will always be an instance of the structure. The code above (that types fields) handles this correctly. Right now the following will be generated for the query from test/support/codegen/edgeql/types/select_result.edgeql:

defmodule Result do # ... defmodule OlAB do @derive Jason.Encoder defstruct [:id] @type t() :: %__MODULE__{id: Tests.Codegen.Queries.Types.SelectResult.uuid()} | nil # this is not required end @type t() :: %__MODULE__{ id: Tests.Codegen.Queries.Types.SelectResult.uuid(), rp_a_str: String.t(), rp_b_str: String.t(), rp_c_str: String.t(), rp_d_str: String.t(), rp_f_str: String.t(), ol_a: OlA.t() | nil, ml_a: list(MlA.t()), ol_a_b: OlAB.t() | nil, # this is correct ml_a_b: list(MlAB.t()) } # ... end

nsidnev · 2023-10-06T07:58:00Z

priv/codegen/templates/_object.eex

+                    render_set: @render_set,
+                    module_name: @module_name
+                ) %>
+            end;


This comment applies to most of the changes in templates. I noticed that you use ; at the end of statements, but you don't need that in Elixir. Can you please remove it here and in other places?

nsidnev · 2023-10-06T08:11:37Z

lib/edgedb/edgeql/generator.ex

-        should_render_type_for_shape: complex_shape?(raw_shape),
-        result_type: (complex_shape?(raw_shape) && "result()") || rendered_shape,
+        should_render_type_for_shape: rendered_schema && complex?,
+        cardinality_to_function: @cardinality_to_function,


I think this is now useless, right? Since the template code checks the cardinality itself

nsidnev · 2023-10-06T08:23:29Z

lib/edgedb/edgeql/generator.ex

-        query_function: @cardinality_to_function[query.result_cardinality],
-        should_render_type_for_shape: complex_shape?(raw_shape),
-        result_type: (complex_shape?(raw_shape) && "result()") || rendered_shape,
+        should_render_type_for_shape: rendered_schema && complex?,


This code probably is wrong when handling the case where an empty object is returned from the database. The following code for test/support/codegen/edgeql/types/insert_f_named.edgeql will be generated for query/3 and query!/3:

@spec query( client :: EdgeDB.client(), args :: args(), opts :: list(EdgeDB.query_option()) ) :: {:ok, Result.t()} | {:error, reason} when reason: any() @spec query!( client :: EdgeDB.client(), args :: args(), opts :: list(EdgeDB.query_option()) ) :: Result.t()

But the Result module itself won't be generated. In this case, the structure should also be generated, but it will have no fields

nsidnev and others added 2 commits September 24, 2023 14:25

implement generation of EdgeQL queries modules

779d897

feat: structure generation

309944d

raddevon requested review from nsidnev and removed request for nsidnev September 26, 2023 12:39

raddevon assigned nsidnev Sep 26, 2023

raddevon added the enhancement New feature or request label Sep 26, 2023

deviprsd and others added 7 commits September 26, 2023 10:45

feat: schema transformation (non optimized)

1e12527

fix credo warnings

207b846

Merge branch 'edgeql-generation' into edgeql-generation

f8b99ff

feat-wip: do_query

62302f9

chore: clean up and bring to sync

0b19e66

wip: full typed module name, better result type

9d1f7da

feat: set improvements

f43bba2

deviprsd marked this pull request as ready for review September 29, 2023 06:27

nsidnev and others added 2 commits September 29, 2023 20:11

implement generation of EdgeQL queries modules

eafab89

fix credo warnings

198229a

nsidnev force-pushed the edgeql-generation branch 5 times, most recently from 3a4efaf to fcf5085 Compare September 30, 2023 00:37

update CI

1034cea

nsidnev force-pushed the edgeql-generation branch from fcf5085 to 1034cea Compare September 30, 2023 00:58

deviprsd closed this Oct 6, 2023

deviprsd force-pushed the edgeql-generation branch from f43bba2 to 1034cea Compare October 6, 2023 06:12

Merge branch 'edgeql-generation' of github.com:deviprsd/edgedb-elixir…

5bc239f

… into edgeql-generation

deviprsd reopened this Oct 6, 2023

nsidnev requested changes Oct 6, 2023

View reviewed changes

deviprsd and others added 2 commits October 6, 2023 23:47

chore: clean up merge

368207c

use error renderer for codegen

56af269

nsidnev added 2 commits October 9, 2023 20:13

wrap enum values into quotes

f3687c3

apply formatter

f73480c

deviprsd closed this Oct 10, 2023

deviprsd force-pushed the edgeql-generation branch from 368207c to f73480c Compare October 10, 2023 03:26

chore: sync master

f9443c7

deviprsd reopened this Oct 10, 2023

nsidnev force-pushed the edgeql-generation branch from f73480c to 68c149a Compare October 18, 2023 19:05

nsidnev force-pushed the edgeql-generation branch from 68c149a to 8575b89 Compare November 2, 2023 00:10

nsidnev force-pushed the edgeql-generation branch 9 times, most recently from 3cd967d to 2a56d07 Compare March 1, 2024 17:50

nsidnev force-pushed the edgeql-generation branch from 2a56d07 to ff48c7c Compare May 26, 2024 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: edgeql structure generation #168

feat: edgeql structure generation #168

deviprsd commented Sep 26, 2023 •

edited

Loading

nsidnev left a comment

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023 •

edited

Loading

nsidnev Oct 6, 2023 •

edited

Loading

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023

nsidnev Oct 6, 2023 •

edited

Loading

feat: edgeql structure generation #168

Are you sure you want to change the base?

feat: edgeql structure generation #168

Conversation

deviprsd commented Sep 26, 2023 • edited Loading

nsidnev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsidnev Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

nsidnev Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsidnev Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

deviprsd commented Sep 26, 2023 •

edited

Loading

nsidnev Oct 6, 2023 •

edited

Loading

nsidnev Oct 6, 2023 •

edited

Loading

nsidnev Oct 6, 2023 •

edited

Loading