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.

Sign up for GitHub

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

Jump to bottom

Add a Date decoder to the pg adapter #51483

Merged
merged 1 commit into from Apr 30, 2024
Merged

Add a Date decoder to the pg adapter #51483

merged 1 commit into from Apr 30, 2024
+17 −0

Conversation

JoeDupuis
Copy link
Contributor

Fix #51448

Add a Date decoder to the pg adapter to type cast dates at the connection level

This would type cast columns of type date to ruby Date when running a raw query through ActiveRecord::Base.connection.select_all.

Before:

ActiveRecord::Base.connection.select_value("select '2024-01-01'::date").class 
#=> String

After:

ActiveRecord::Base.connection.select_value("select '2024-01-01'::date").class 
#=>  Date

While I don't think we'd want to to set a type cast expectation at this level (not all adapters have dates), this would brings the PG adapter to parity (for dates) with the Mysql2 adapter.

We already convert timestamp, it would makes sense to also convert dates.

I wasn't sure if I should add a test. I am thinking that a test sets an expectation about type casting at the adapter level.
This changes a public API, albeit a pretty low level one, but it seems like we don't test the other types either. I removed timestamps + float/numeric and all tests still pass.

I added a message in the changelog entry to make sure it doesn't burn anyone.

Unsure if we should merge this. Impact radius is large, but after digging into it and running the tests, it looks pretty benign.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@JoeDupuis JoeDupuis force-pushed the add-date-text-decoder-postgresql-adapter branch from ea64815 to 92f7d37 Compare April 4, 2024 00:29
@JoeDupuis JoeDupuis mentioned this pull request Apr 5, 2024
Closed
@byroot
Copy link
Member

byroot commented Apr 17, 2024

Could you add a test? Particularly one that make sure it will be parsed correctly when using a model.

@JoeDupuis JoeDupuis force-pushed the add-date-text-decoder-postgresql-adapter branch from 92f7d37 to fc9871d Compare April 30, 2024 02:08
@JoeDupuis
Copy link
Contributor Author

JoeDupuis commented Apr 30, 2024 •
edited

Sorry, I was away last week.

@byroot while looking for a place to write the test, I found those. Do you think they are sufficient?

I've also added a test for the decoding of the date : https://github.com/rails/rails/pull/51483/files#diff-f3109c7be3eb7602954054584dc86a6edc790b68dee0fc36654b427b8b11febfR43-R47

@JoeDupuis
Add a Date decoder to the pg adapter to type cast dates
2c88c80 
at the connection level

Fix rails#51448

Type cast columns of type `date` to ruby `Date` when running a raw
query through `ActiveRecord::Base.connection.select_all`.
@JoeDupuis JoeDupuis force-pushed the add-date-text-decoder-postgresql-adapter branch from fc9871d to 2c88c80 Compare April 30, 2024 02:16
@byroot byroot merged commit 6a38d3a into rails:main Apr 30, 2024
4 checks passed
@ghiculescu
Copy link
Member

ghiculescu commented May 7, 2024 •
edited

On the one hand, this is definitely an improvement 👍

On the other hand, it's almost certainly a breaking change. We had some code that broke from this, where we were manually doing Date.parse on the result of a query and had to remove the parsing, due to a TypeError: no implicit conversion of Date into String. We found this example via tests, but it's hard to audit for, without manually inspecting every selected column in every single manually constructed query. I worry that anywhere that doesn't have sufficient test coverage for this will lead to sudden errors.

Is it worth making this an opt in? I am happy to make a PR if so. @byroot @JoeDupuis

@byroot
Copy link
Member

byroot commented May 7, 2024

It probably should have it's config and load_defaults yeah :/

@JoeDupuis
Copy link
Contributor Author

Shoot 😕 I can write a PR later today.

@JoeDupuis
Copy link
Contributor Author

I am happy to make a PR

@ghiculescu Mind if I do it? That'll teach me about the config/default system. I'll keep that in mind for the next PR 😅

@ghiculescu
Copy link
Member

Go ahead @JoeDupuis ! I can review if you like

JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 8, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
c2d01f6 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 8, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
1ea165b 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
@JoeDupuis JoeDupuis mentioned this pull request May 8, 2024
Open
4 tasks
@JoeDupuis
Copy link
Contributor Author

@ghiculescu I opened the PR: #51763

JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 8, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
b8c3f24 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 8, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
fddc0b8 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 8, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
b4fcbe7 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 9, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
dafe0fc 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 9, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
b2bfd24 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 9, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
153fbaf 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
byroot pushed a commit to JoeDupuis/rails that referenced this pull request May 10, 2024
@JoeDupuis @byroot
Add active_record.postgresql_adapter_decode_dates
401c1d3 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
JoeDupuis added a commit to JoeDupuis/rails that referenced this pull request May 10, 2024
@JoeDupuis
Add active_record.postgresql_adapter_decode_dates
c40b8b7 
to toggle automatic decoding of dates column with the
PostgresqlAdapter.

PR rails#51483 is a breaking change and should have been gated behind a
config.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Postgresql date type is not properly typecasted when using direct SQL queries like select_all
3 participants
@JoeDupuis @byroot @ghiculescu