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

PG set stringtype=unspecified to handle string/uuid edge case #1087

Open
wants to merge 7 commits into
base: 61-stable
Choose a base branch
from

Conversation

imustafin
Copy link

Fixes #1086.

I'm not sure if changing stringtype is a good idea, but tests pass. I am now testing these changes in our projects.

I think that standard Rails with pg is working similarly by letting postgres to infer the needed types.

@@ -62,7 +62,7 @@ def translate_exception(exception, message:, sql:, binds:)
end

def extract_raw_bind_values(binds)
binds.map(&:value_for_database)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something only missing for postgresql?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not sure. This change is not really relevant to the purpose of this PR.

Also, #1088 (comment) reports that this should not be an issue in Rails 6.1.4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah I guess we will not worry about that one. I am trying to get our CI working again. Looks like some changes in bundler and retrieving via git was messed up. Once I get it ok I will rerun this.

@enebo enebo changed the base branch from master to 61-stable July 29, 2021 19:52
@enebo
Copy link
Member

enebo commented Jul 29, 2021

I updated this against 61-stable since against master I am seeing a lot of errors which most likely is a problem with rails HEAD.

@enebo
Copy link
Member

enebo commented Jul 29, 2021

@imustafin unfortunately we have a few consistent errors but this particular one looks like it added some new one related to prepared arity mismatch. Any chance stringtype is involved here?

@dr-itz If the above is not really an issue for this PR do you have an opinion here? Also @kares :)

https://app.travis-ci.com/github/jruby/activerecord-jdbc-adapter/jobs/527824796

@imustafin
Copy link
Author

@enebo which tests are now failing comparing to 61-stable? I can't seem to find them.

I'll take a look at this PR (hopefully by Monday). Do you plan to make CI green soon on 61-stable?

@enebo
Copy link
Member

enebo commented Jul 29, 2021

@imustafin in the link above it is the tests like this:

"ActiveRecord::ConnectionAdapters::PostgreSQLAdapterTest#test_exec_with_binds:

ActiveRecord::StatementInvalid: ActiveRecord::JDBCError: org.postgresql.util.PSQLException: ERROR: bind message supplies 0 parameters, but prepared statement "" requires 1"

I did not see these in the head build for 61-stable.

@imustafin
Copy link
Author

Co-authored-by: Karol Bucek <kares@users.noreply.github.com>
@enebo
Copy link
Member

enebo commented Jul 30, 2021

@imustafin yeah. I just merged @kares review comment since it makes sense to honor settings. This will start a new CI run and we can see if we still see those couple of errors with this change or they are intermittent.

@Faq
Copy link

Faq commented Feb 26, 2022

What is the status of this PR?

@imustafin
Copy link
Author

I'm still here but haven't used activerecord-jdbc-adapter for a long time.

Maybe I can rebase this PR and we can resurrect the discussion.

@Faq do you want this to be merged? What is you use case? Is it similar to #1086?

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.

PG strings in arrays are casted to UUIDs when not needed
4 participants