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

Update dependencies with warnings #723

Merged
merged 10 commits into from
Jan 13, 2025
Merged

Conversation

TylerWitt
Copy link
Contributor

I was digging around to ensure that JSON is supported, and saw a lot of deprecations when compiling deps.

I didn't bump decimal even though it also has warnings because I'm not sure if we want to bump to v2 only (now that Jason supports v2, which I'm assuming is why v1 was also supported), or if we want to keep testing the v1 version of the library for some reason.

I know it's a super minor change, though. Feel free to close!

Bumps db_connection to drop connection, since connection raises a bunch of deprecation warnings for Elixir 1.18
@josevalim
Copy link
Member

Thank you, although CI seems to be unhappy :)

@TylerWitt
Copy link
Contributor Author

Thank you, although CI seems to be unhappy :)

Yeah, I'm trying to check it out. I'm assuming it has to do with the db_connection changes.

@TylerWitt
Copy link
Contributor Author

The 1.17 branch seems to be an error that might have been fixed by Elixir 1.17.2

15:08:50.775 [error] Failure while translating Erlang's logger event\n** (MatchError) no match of right hand side value: 
%{callback_mode: :handle_event_function, client_info: :undefined, label: {:gen_statem, :terminate}, log: [], 
modules: [DBConnection.Connection], name: #PID<0.1902.0>, postponed: [], queue: [internal: {:connect, :init}], 
reason: {:error, %ArgumentError{message: \"expected :search_path to be a list of strings, got: \\\"public, extension\\\"\"}

@josevalim
Copy link
Member

Yes, let's bump v1.17 as well?

@TylerWitt
Copy link
Contributor Author

From what I can tell, this line in db_connection makes the genstatem pattern incompatible with Elixir versions before 1.17.2 when raise is used during a callback, since it will enter gen_statem's internal terminate, which generates the log format that was fixed in 1.17.2.

Assuming that's correct, raising in a gen_statem callback when the Elixir version is lower than 1.17.2 means the errors will crash the logger.

What should we do in terms of Postgrex? Do we

  • Bump the Elixir version of this library to be a min of 1.17.2?
  • Prevent Postgrex from using the db_connection versions that implement genstatem?
  • Revert genstatem in db_connection, and continue maintaining the connection library?
  • Patch the logger fix for Elixir into each of the Elixir minor versions?

Or is this simply not a big deal?

@josevalim
Copy link
Member

Users should update from 1.17.2, as guarantee bug compatibility everywhere else is not worth it, IMO. :)

@josevalim
Copy link
Member

To expand a bit more, other libraries with gen_statem will have the same issue, so the correct thing is to update Elixir. :) Which is what we should do on CI here!

@TylerWitt
Copy link
Contributor Author

@josevalim This might also be an appropriate way to still support older versions elixir-ecto/db_connection#320

I'm not sure of the downstream implications (yet) since it has been a couple years since I've converted a gen_fsm project to gen_statem, and the side effects of postgrex + db_connection's nested nature is still new to me.

mix.exs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@TylerWitt
Copy link
Contributor Author

I added a conditional skip on the test by parsing the Elixir version, which feels like a huge hack, but it lets CI still test older versions while skipping the test (but still running it on newer versions of Elixir/otp).

test/postgrex_test.exs Outdated Show resolved Hide resolved
@josevalim josevalim merged commit 9748fcb into elixir-ecto:master Jan 13, 2025
9 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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