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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add postgres schema support #518

Merged
merged 6 commits into from Aug 16, 2015

Conversation

Projects
None yet
8 participants
@danieltdt
Contributor

danieltdt commented Oct 3, 2014

Hello,
I've been using knex for many projects and I see that it didn't have a good support for postgres schemas. So, I created this pull request adding a new method .using which allows to scope table names within a schema (and adding the schema to the metadata queries).

Since many databases allows the "qualified name" structure for table names, I decided to put it into the generic objects builder and compiler, since it does not broken any API.

I added a few tests also and it seems to run fine (I don't have oracle/mariadb to run integration tests 馃槥 ).

So, do you guys think it is good to merge?

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Oct 3, 2014

Thanks for the PR @danieltdt - going to wait on merging this right away to compare some of these changes with #316 - I was holding off on that because I wanted to make sure the API was the clearest possible. @tkellen you seem to utilize more of the full PG feature set than I, any opinions on either of these PR's.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Oct 3, 2014

I think both PR can be merged with small changes. This PR only includes .using and changes some methods to be aware of the current schema. The #316 adds more methods, like searchPath and hasSchema (the createSchema and dropSchema methods are already implemented on current master).
One thing that I would change is remove searchPath(newPath) from the schema API, since it uses the set command and, AFAIK, it is defined per connection. I would move it to the connection object, like:

{
  client: 'pg',
  connection: 'postgress://postgres@localhost:5432/knex',
  searchPath: 'myschema, public'
}
@tkellen

This comment has been minimized.

Collaborator

tkellen commented Oct 3, 2014

Agreed 100% on setting the search path in the connection.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Oct 3, 2014

Added searchPath on config.

@meredian

This comment has been minimized.

meredian commented Nov 2, 2014

Any news on including schema support? About this, or #316 Issue

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Mar 6, 2015

So... Any thoughts about it?

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Mar 6, 2015

Sorry for keeping you waiting for so long. I appreciate your patience. The steps to getting this merged are:

  • Reviewing #316 and making sure this has parity w/ the schema functionality introduced there
  • Rebasing to squash and remove the merge commit

Re #316, I'm not concerned w/ the schema builder methods for creating new schemas or modifying. Just want to make sure that anything that any good ideas with respect to using schemas introduced there are reflected here. I'll work on extracting the schema builder functionality from #316 separately.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Mar 6, 2015

Ok! I'll check this later at home.

@whitelynx

This comment has been minimized.

Contributor

whitelynx commented Jul 9, 2015

I'd love to see this rebased so it can be merged.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Jul 9, 2015

lol I totally forgot about this PR 馃檴
I'll try to rebase it now...

@standyro

This comment has been minimized.

standyro commented Jul 23, 2015

馃憤 seconded on getting this pull request rebased and in

@meredian

This comment has been minimized.

meredian commented Aug 7, 2015

Can be any help provided for branch merging? Waiting this patch already for 10 months :)

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 7, 2015

Hi @danieltdt. Looks like there's a few people waiting on this. Are you still interested in getting it merged? If you can satisfy @bendrucker's requests, I'll accept the PR.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Aug 8, 2015

Sorry about the delay, guys... The codebase of knex has changed deeply and the rebase is not so simple as I expected. I had given up this PR but, since people seems interested on it, I'll try to rebase it on this weekend.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Aug 10, 2015

Please, take a look and let me know if it is good to merge :)

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 10, 2015

Hi @danieltdt, thanks for coming back to this.

I am primarily focused on Bookshelf, so I do not know the internals of knex very well. Also I've never worked with multiple schemas before in Postgres, but I checked the docs and the concept is simple enough. Your changes look very clean superficially, and the tests seem good, but beyond that I can't really comment on the correctness of the implementation. I'm happy to defer to your judgement.

Sorry I didn't mention this in the last comment, but I think this really requires documentation. Would you be able to also update index.html?

Also, I'm not sold on the method name using. USING is already a keyword in PostgreSQL - but seemingly not in the case of specifying schema(?). I think schema() or something similarly explicit might be more appropriate. What are your thoughts?

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Aug 10, 2015

Hey @rhys-vdw,
When I was implementing that, I really could not find a better name for that method xD
My first thought was schema() but I think knex.schema.schema('myschema').table(...) would be a bit strange... So I thought about using, but it is already a keyword in postgres (and all others DBMSs too) but it is already implemented on knex (https://github.com/tgriesser/knex/blob/master/src/query/joinclause.js#L45) under a specific API.
I decided to keep using method because I could not find out any other meaningful name for that. If you think schema() is good for knex API, I'm ok renaming it.
Once we decide the method name, I will update the docs.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 10, 2015

I think knex.schema.schema('myschema').table(...) would be a bit strange...

True. And also would it not conflict with knex.schema as it is on the QueryBuilder also?

I still fear using is too ambiguous. When scanning the documentation I think I'd be looking for 'schema'. Also, it may be that USING keyword needs to be added to QueryBuilder for some reason in the future.

However, I can't think of anything sufficiently terse. usingSchema or withSchema perhaps? Too awkward?

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Aug 10, 2015

knex.schema use SchemaBuilder for DDL commands. QueryBuilder is only for DML queries. They should never conflict.

Maybe withSchema is a good option! May I rename to it?

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 11, 2015

@danieltdt: Yes, please. :-) Sorry for the slow response.

@danieltdt

This comment has been minimized.

Contributor

danieltdt commented Aug 14, 2015

@rhys-vdw Done! (sorry about slow commits 馃檲)

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 16, 2015

Looks solid. Nice one. 馃憤

rhys-vdw added a commit that referenced this pull request Aug 16, 2015

@rhys-vdw rhys-vdw merged commit 255daec into tgriesser:master Aug 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment