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 support for PostgreSQL types: UUID, Array, JSONB and Money #84

Merged
merged 7 commits into from
Oct 12, 2016

Conversation

jodosha
Copy link
Collaborator

@jodosha jodosha commented Sep 6, 2016

@@ -0,0 +1,82 @@
require 'rom/sql/types/pg'
Copy link
Member

Choose a reason for hiding this comment

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

these pg extensions should be opt-in, inferrer class can be set in relation, so we can use different inferrers for different dbs by calling schema_inferrer ROM::SQL::Schema::PGInferrer in relation class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How so? Can you please show me? Thanks 👍

Copy link
Member

Choose a reason for hiding this comment

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

something like this https://gist.github.com/flash-gordon/901b94a9af623952f1babca50d7b8cd9 /cc @solnic can have some thought on this too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But then the problem becomes to explicit setup it.

Copy link
Member

Choose a reason for hiding this comment

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

@jodosha well, technically, this is a manageable thing to do—to load a proper extension based on what type of connection we've got, though it'll require some rework around the inferrer infrastructure. /cc @solnic

@flash-gordon
Copy link
Member

I'm also afraid that insertion of array values wouldn't work, at least this is what I came across today trying to use an array type in a manually maintained schema. I'll have a deeper look into this tomorrow

@jodosha
Copy link
Collaborator Author

jodosha commented Sep 7, 2016

@flash-gordon

I'm also afraid that insertion of array values wouldn't work

You're right, it wraps the array like this: ('ruby'), but it should be {'ruby'}.

@jodosha
Copy link
Collaborator Author

jodosha commented Sep 7, 2016

@flash-gordon It looks like Dry::Types::Definition#call doesn't get called. I also added debuggers to all the definitions of pg_array in Sequel: none of them gets called.

@solnic
Copy link
Member

solnic commented Sep 28, 2016

We can give it a shot. The only potential problem we may face is lack of
some info in the moment when we need it, since connection setup is not
available globally, we'd have to provide it to the inferer when relations
are being created.

On 18 September 2016 at 10:01:39, Nikita Shilnikov (notifications@github.com)
wrote:

@flash-gordon commented on this pull request.

In lib/rom/sql/schema/column_inferrer.rb
#84:

@@ -0,0 +1,82 @@
+require 'rom/sql/types/pg'

@jodosha https://github.com/jodosha well, technically, this is a
manageable thing to do—to load a proper extension based on what type of
connection we've got, though it'll require some rework around the inferrer
infrastructure. /cc @solnic https://github.com/solnic


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#84, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAEKvKlp4ABcWNBw5cr6NUFofwe4Pzpks5qrURDgaJpZM4J2AS8
.

@gotar
Copy link
Member

gotar commented Sep 30, 2016

what with other db systems (other that pg?) for me it should be some kind of plugin, you can require manually.

@flash-gordon
Copy link
Member

@gotar I'm working on this atm, going to have a preview this weekend

@flash-gordon
Copy link
Member

@jodosha I force pushed the branch onto your repo, now I'm going to move some stuff here and there, then I'll let you know when it's ready or if I get any questions. The main thing, you probably already know about, is that I want to wrap all PG-specific code into an automatically activated extension

JSON = Array | Hash
JSONOp = Dry::Types::Definition
.new(Sequel::Postgres::JSONOp)
.constructor(Sequel.method(:pg_json))
Copy link
Member

Choose a reason for hiding this comment

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

Why is it :pg_json method?

Copy link
Member

Choose a reason for hiding this comment

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

See Sequel's docs. TLDR Sequel allows to pass various values as a JSON(B) value but they are must be wrapped with pg_json call beforehand.

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha

@solnic
Copy link
Member

solnic commented Oct 12, 2016

This looks good. Pls merge <3

@flash-gordon flash-gordon merged commit 61450a2 into rom-rb:master Oct 12, 2016
@flash-gordon
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.

4 participants