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

Added minimal implementation of PGconn with unescape_bytea #655

Merged
merged 1 commit into from
Jun 8, 2015

Conversation

donv
Copy link
Member

@donv donv commented Jun 7, 2015

To fix #651 and #652 .

@donv donv added this to the 1.4.0 milestone Jun 7, 2015
@donv
Copy link
Member Author

donv commented Jun 8, 2015

Before:

380 tests, 608 assertions, 4 failures, 21 errors, 2 pendings, 72 omissions, 0 notifications

After:

380 tests, 639 assertions, 4 failures, 17 errors, 2 pendings, 72 omissions, 0 notifications

4 errors fixed. OK to merge?

@prathamesh-sonpatki
Copy link
Contributor

👍

donv added a commit that referenced this pull request Jun 8, 2015
Added minimal implementation of PGconn with unescape_bytea
@donv donv merged commit 5a9978c into master Jun 8, 2015
@donv donv deleted the test-pgconn branch June 8, 2015 11:43
@kares
Copy link
Member

kares commented Jun 8, 2015

Thanks Uwe, looks good however with jruby-pg around it's quite ugly to pollute their name-space ... we did moneky-patch AR previously so would you mind if I change that ?

also I'm not sure why you put up the 1.4 target ... I'd like to put it on 1-3-stable and then merge here so that it lands in next stable release (for 1.4 I would also move this code into native), thoughts?

@donv
Copy link
Member Author

donv commented Jun 8, 2015

Hi @kares !

Thanks for looking at this. Feel free to change as you see fit 😄 and put on the 1-3-stable as well. I was unsure if 1-3-stable targeted AR 4.2.

I got an error using schema_plus without any implemented PGconn because the OID implementation for PostgreSQL is loaded then. Not sure if this is intended, but it seems ARJDBC aims to use as much standard AR as possible, and AR uses PGconn directly.

@kares
Copy link
Member

kares commented Jun 8, 2015

oh, you guys merged that already - well just go for it than - I do not have time to "play" around AR 4.2 compatibility hacks just be aware when introducing another :) ... planning a 1.4 pre-release from master.

@kares
Copy link
Member

kares commented Jun 8, 2015

@donv OID was completely re-invented previously, AR "promised" mixins will be shareable ...but it's not :(

@donv
Copy link
Member Author

donv commented Jun 8, 2015

I'd like everyone to play nice together, but it seems jruby-pg and ARJDBC are exclusive, right? They should not be used together? AR+ARJDBC or AR+jruby-pg, right?

@kares
Copy link
Member

kares commented Jun 8, 2015

why would they be exclusive? ... I mean for AR they might be, but pg is not made solely for AR ...

@kares
Copy link
Member

kares commented Jun 8, 2015

... you can hide the PGConn under OID module (for now) ... it should work since there's no ::PGConn

@donv
Copy link
Member Author

donv commented Jun 8, 2015

Oh. I'm learning, then :) How would you use jruby-pg with ARJDBC?

I'll move the PGconn class below the ActiveRecord::ConnectionAdapters::PostgreSQL::OID module.

@kares
Copy link
Member

kares commented Jun 8, 2015

I mean for AR they might be, but pg is not made solely for AR ...

... one can access DBs in different ways (along side with AR) - which is probably uncommon but possible

@kares kares removed this from the 1.4.0 milestone Jun 9, 2015
@kares kares added the rails-4.2 label Jun 9, 2015
kares added a commit that referenced this pull request Jun 9, 2015
to fix #651 as an unfortunate work-around for (non-shareable) AR internals #652
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.

Problem reading postgresql table with binary/bytea column
3 participants