search

fuelen / ecto_dev_logger

An alternative logger for Ecto queries
Apache License 2.0
151 stars 16 forks source link

support serialization of custom types #7

Closed trbngr closed 2 years ago

trbngr commented 2 years ago

Hello,

This is a great library! Thanks for your work.

We have some custom types that ecto_dev_logger was not really digging.

A Postgrex.Range was making the telemetry handler error out and detach.

Instead of trying to keep up with an endless amount of parameters to support, or even just simply inspecting unsupported parameter types, I added support for plugging in a custom ParameterSerializer.

The application config is parsed when invoking DevLogger.install and passed down to all stringify_ecto_params as opts. When we can not find a match to serialize a known parameter, we reach out to the ParameterSerializer in configuration -- or ParameterSerializer.Default as a fallback -- to serialize unknown parameters.

The ParameterSerializer.Default will simply raise an exception. This matches to the current behaviour but in a nicer fashion. A ParameterSerializer.Error instead of a FunctionClauseError.

fuelen commented 2 years ago

Hi @trbngr

I'd like to properly understand the problem you're trying to solve.

We have some custom types that ecto_dev_logger was not really digging.

Which custom types are you talking about? Are they Ecto types? Or you're using custom extensions of Postgrex https://github.com/elixir-ecto/postgrex#extensions ?

The changes you're proposing solve the issue that is present only with custom Postgrex extensions.

If you have only Ecto types, then they are eventually dumped into Postgrex types. So, the number of types is not endless and theoretically, we can (and want to) support all or almost all the default types.

Now, about the implementation. The system you've made is basically a hand-rolled implementation of Elixir protocols, and I'd rather use the latter. But the protocol has to be wisely designed to support complex data types, like an array of binaries. I'd be happy if you implement casting to string using protocol, or if you simply add a support for Postgrex.Range instead :)

I appreciate your efforts, but, currently, I can't accept this PR.

trbngr commented 2 years ago

That's understandable.

If you look at the test file, you'll see that I'm using a custom ecto type. Although I just did realize that I'm not using it directly.

After reading your comment, it's pretty obvious, in hindsight, that a protocol would work. I'll give it another go with a protocol when I get a chance.