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

ActiveRecord PostgreSQL adapter: escape range bound values during serialization #39585

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chrisandreae
Copy link
Contributor

@chrisandreae chrisandreae commented Jun 10, 2020

Summary

When formatting the bound values of range types, PostgreSQL quotes the value using double-quotes in certain conditions: if its formatted representation contains parentheses, commas, double quotes, backslashes or whitespace (manual section 8.17.5, 8.16.6). When parsing input, it requires escaping in nearly the same conditions: with the exception of whitespace.

As of #37648, escaped output from Postgres is unescaped during deserialization of range values. However, Rails still does not escape values when serializing for Postgres.

Neglecting to escape doesn't actually cause issues with any of Postgres' pre-defined range types (integer, numeric, time, date), because none of them feature values that strictly require escaping on input (they are only escaped on output due to whitespace). However it's possible to define range types for which this would be an issue: for example, ranges defined over strings, enum values, or arrays.

This PR provides an implementation of bound escaping in ActiveRecord::ConnectionAdapters::PostgreSQL::Quoting#type_cast_range_value.

Questions about the proposed implementation:

  • What types are allowed in the return value of type_cast? In test runs, I see Integer and String. Is it safe to assume that any complex types that would require escaping will have already have been serialized to a string?
  • Is the return value of type_cast guaranteed to be unaliased? If so the call to gsub could be changed to operate in place.

@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 621797b to ce86eb1 Compare June 10, 2020 05:10
chrisandreae added a commit to chrisandreae/rails that referenced this pull request Jun 11, 2020
PostgreSQL uses nested quoting on output for values inside range or composite
types under certain conditions(*), which include the format for timestamps.
ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Range doesn't handle this in
 #cast_value.

(*) If their string representation contains parentheses, commas, double quotes,
backslashes or whitespace (manual section 8.17.5, 8.16.6).

This results in a string containing nested quotes being passed to
AR::CA::PostgreSQL::OID::DateTime, which causes two problems:
 * The special-case handling for BCE dates is bypassed, resulting in incorrect
   positive instead of negative year values.
 * ActiveModel::Type::DateTime's fast_string_to_time parser can't be used and
   falls back to Date._parse

Note that this PR only implements the escaped range bound format when parsing
Postgres output, not when serializing values for Postgres. Postgres also
requires its input to be escaped in the same cases, with the exception of
whitespace. Fortunately, none of Postgres' pre-defined range types use any
characters requiring escaping other than whitespace. See Rails PR rails#39585 for the
serialization side.
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch 2 times, most recently from adccf27 to 4ee7336 Compare June 13, 2020 04:34
@rails-bot
Copy link

rails-bot bot commented Sep 11, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Sep 11, 2020
@rails-bot rails-bot bot removed the stale label Sep 11, 2020
@rails-bot
Copy link

rails-bot bot commented Dec 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 10, 2020
@rails-bot rails-bot bot removed the stale label Dec 12, 2020
Base automatically changed from master to main January 14, 2021 17:01
@rails-bot
Copy link

rails-bot bot commented Apr 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 14, 2021
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 87c3871 to 52e7ce1 Compare April 15, 2021 02:20
@rails-bot rails-bot bot removed the stale label Apr 15, 2021
@rails-bot
Copy link

rails-bot bot commented Jul 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jul 14, 2021
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 52e7ce1 to 67f685b Compare July 14, 2021 03:13
@rails-bot rails-bot bot removed the stale label Jul 14, 2021
@chrisandreae
Copy link
Contributor Author

Is there anything I could do to progress this?

@rails-bot
Copy link

rails-bot bot commented Oct 12, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Oct 12, 2021
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 67f685b to 52d0ec1 Compare October 14, 2021 01:30
@rails-bot rails-bot bot removed the stale label Oct 14, 2021
@rails-bot
Copy link

rails-bot bot commented Jan 12, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jan 12, 2022
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 52d0ec1 to 82252c9 Compare January 13, 2022 07:20
@rails-bot rails-bot bot removed the stale label Jan 13, 2022
@rails-bot
Copy link

rails-bot bot commented Apr 13, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 13, 2022
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 82252c9 to 1b6c39b Compare April 15, 2022 03:55
@rails-bot rails-bot bot removed the stale label Apr 15, 2022
Postgres requires range bound values containing parentheses, brackets, commas,
double quotes, or backslashes to be escaped.
@chrisandreae chrisandreae force-pushed the postgresql-escaped-range-bounds branch from 1b6c39b to f925656 Compare October 3, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant