Do not assume that the primary key is "id"

[aesmail]

Need a way to figure out the primary key.
Might also need to check if the primary key is a set of fields and not just a single field.

[wstucco]

You can retrieve the primary key of a Module schema using

Module.__schema__(:primary_key)

https://hexdocs.pm/ecto/Ecto.Schema.html#module-reflection

[aesmail]

Hello @wstucco
You're right, but this potentially could return a list of fields (composite primary keys). In this case, too much of kaffy's internals must change in order to support this. I'm not sure if this is actually worth supporting in the time being.

First step is to support single-field custom primary keys. Later, I'll see if supporting composite primary keys is feasible or not.

[cultofmetatron]

+1 this, would be great for those of us incorporating phoenix on top of legacy databases

[nullpilot]

Hi @aesmail ! I have have been using Kaffy for a small project that has some joiner tables, where I left off the id and used a composite primary key instead. As Kaffy doesn't currently support either, I thought it was a good opportunity to look into it a bit and see if I could come up with a working solution. It's not done yet, but it's at a point where basic navigation and modification seem fine, and it would be great to get some feedback, to see if it's worth moving forward with.

The approach used is to introduce serialize_id/2 and deserialize_id/2 in the admin module, which by default just join/split all primary key values with a ".", which is then used for the routes and some form values. This way the routing logic doesn't have to change at all, and generally there should be no visible changes for schemas that worked previously.

The major places where a hard coded :id was assumed have been rewritten to use the primary key list returned by the schema and the new (de)serialized id's to match.

Would love to hear your thoughts on this.

[aesmail]

Hey @nullpilot,
Thanks a lot for taking the time to give this issue a shot. I like the idea you're proposing and I'm glad you're sharing the initial work because Kaffy is moving to a new direction with the next release. I'll try to use your solution with what I have locally and provide feedback in a few days. Also, since I'm not sharing a lot about what's coming next, and this might make it harder for people to contribute effectively, I'll open an new issue that will explain what the major change in v0.10.0 is all about.

I'll keep your PR open for now, but I expect that it will be extremely valuable in the coming days.

[wstucco]

@nullpilot @aesmail

Can I suggest to not to use the dot as a separator?

In PostgreSQL, for example, this code is valid

CREATE TABLE test (
  "id.id" INT
);
INSERT INTO test ("id.id") VALUES (1);

I think using Json or Erlang ETF as serialization format would be better.

Something along the lines of

[<primary keys>]
|> :erlang.term_to_binary()
|> Base.encode64()

or

Jason.encode([<primary keys>])

[nullpilot]

@aesmail: Thanks for checking it out, glad to hear it might be helpful. I'll keep an eye on the repo for what you have coming up.

@wstucco: The serialization as is only joins together the values, not the names of the keys, so in the case of uint/serial/uuid there shouldn't be a problem. In the case of strings or some other type that could introduce ambiguity, the methods could be overwritten to use one of the methods you suggest with a final Base.url_encode64/2.

In my local example the route currently ends up looking something like this for a binary-id composite:

http://localhost:4000/admin/listings/product_tags/bcb391ff-8ce6-47e5-8d1d-068173361f57.eeed2940-644a-4b0d-ae40-919dc670a7a0

[wstucco]

@nullpilot

The serialization as is only joins together the values

Sorry, my fault, I was only trying to show that the dot can appear in many places one wouldn't think to find them.

I think safer defaults will avoid weird bugs in the future.

In the case of composite keys there's a very high chance they won't be of a safe base type.

The problem is that if the keys have unsafe values the serializer will produce the wrong result, but the deserializer won't fail, it will simply emit the wrong key values.

(1)> Enum.join([1.2, 3.4], ".")
"1.2.3.4"

(2)> String.split("1.2.3.4", ".") |> Enum.zip(["id", "sub_id"])
[{"1", "id"}, {"2", "sub_id"}]

while ETF returns the correct result

(3)> :erlang.term_to_binary([1.2, 3.4]) 
     |> Base.encode64() 
     |> Base.decode64!() 
     |> :erlang.binary_to_term() 
     |> Enum.zip(["id", "sub_id"])
[{1.2, "id"}, {3.4, "sub_id"}]

[nullpilot]

That's a good point, I like the solution as well. I do think there is some value in "readable" URLs that contain the actual IDs where possible (maybe even non-primary, unique slugs in the future, for example) - but having a more encompassing default is absolutely worth the consideration. Since the setup is quite flexible, it could even be combined to use a "best effort" approach, depending on the types of primary keys present in a schema.

[wstucco]

I do think there is some value in "readable" URLs that contain the actual IDs

Me too!

Maybe replacing the dot with a less common character (for example ^, ~, !) could reduce the risk of a collision to a minimum

[nullpilot]

That sounds viable. There aren't many options: - . _ ~ are the only URL-safe characters with no reserved meaning. Out of those, ~ seems like the most fitting to me and goes along with your suggestion. It's not currently an issue, but ~ is not valid in HTML id and name attributes, which could lead to conflicts with the autogenerated forms down the line. The only alternative that comes to mind would be :, as it works in URLs (e.g. Wikipedia) and is valid as id/name.
On top of that the ETF solution could be used if one of the pk types isn't :id or :binary-id maybe, or featured in the docs as example for overwriting the serialization logic.

For now, let's see what @aesmail has in store for us 😃

[aesmail]

@nullpilot @wstucco interesting discussion.

So you guys have some perspective, here's the gist of what's coming:
I'm trying my best to remove the dependency on ecto and introduce "data adapters".

This way, Kaffy could potentially work with any data source (RDBMs, APIs, NoSQL, csv files, etc).
Kaffy will initially ship with the Ecto adapter by default to keep backwards compatibility.
There will be a common interface to interact with data sources.
Adapters will give Kaffy the data and tell it how to render/manipulate it.

It's trickier than I thought, but we're getting there slowly.

Let me know if you guys have any feedback or comments.
Once I have a workable version, I'll create a new branch for it and you'll be able to see it more clearly.

[nullpilot]

That sounds ambitious, but would be something with many use cases, beyond Kaffy possibly.

Nothing specific comes to mind to comment on for now, but I can see how this issue among others gets some extra depth with this idea. Once it's at a point where it can be tested I'll give it a go!

Thanks for sharing.

[zachdaniel]

@aesmail Once you have a branch for that, I can get started on setting up the ash_kaffy extension :D

[santif]

FWIW

We are using ReactAdmin, which doesn't have support for composite IDs. To circumvent this limitation we use a single param containing the composite ID serialized to JSON and url encoded using base64.

For example:

iex> composite_id = %{"author_id" => 12345, "catalog_id" => "892a081c-f80e-4d08-ad6b-00e5b1749634"}
%{"author_id" => 12345, "catalog_id" => "892a081c-f80e-4d08-ad6b-00e5b1749634"}

iex> uid = composite_id |> Jason.encode!() |> Base.url_encode64(padding: false)                    
"eyJhdXRob3JfaWQiOjEyMzQ1LCJjYXRhbG9nX2lkIjoiODkyYTA4MWMtZjgwZS00ZDA4LWFkNmItMDBlNWIxNzQ5NjM0In0"

# The URL looks like: http://my-server/api/v1/my-resource/eyJhdXRob3JfaWQiOjEyMzQ1LCJjYXRhbG9nX2lkIjoiODkyYTA4MWMtZjgwZS00ZDA4LWFkNmItMDBlNWIxNzQ5NjM0In0

iex> uid |> Base.url_decode64!(padding: false) |> Jason.decode!()                                  
%{"author_id" => 12345, "catalog_id" => "892a081c-f80e-4d08-ad6b-00e5b1749634"}

[ghenry]

Hi all,

Where are we with using uuid / binary_id as a primary key? Is that supported?

Thanks.