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

reorganize sql manager and add CockroachDB support #87

Merged
merged 14 commits into from Nov 10, 2017

Conversation

Projects
None yet
4 participants
@zikes
Copy link
Contributor

commented Nov 1, 2017

Our company needed CockroachDB support, which is very similar to PostgreSQL. CRDB uses the same wire protocol as Postgres, and the SQL format is mostly the same. In an the attempt, I ran into some complications that motivated me to reorganize the SQL manager to be more flexible, if a little more verbose.

Database-specific SQL has been moved to a separate databases.go file, which has a map of supported databases and their configuration. This should allow future databases to be added to the manager externally, as the Databases map is exported. I also copied the SQL that happens to match between databases, rather than store them separately and referencing that, to increase readability.

@aeneasr
Copy link
Member

left a comment

Thank you for the changes. I'm not sure I want cockroach flavored SQL added in the main repository, as it has to be maintained officially and I have to write separate migration routines for a technology I don't use & know.

One path to resolve this would be to have a map[string]StatementConfig in SQLManager that replaced the global Databases reference. That way it would be possible to compose the SQLManager with your own, custom schema without adding it to the main repository.

However, you're addressing some things which I like. Before merging, please address the mentioned things. Depending on how to proceed, some of them might be ignored (like keeping the export of Database).

Let me know what you think.

for _, v := range relations {
for _, template := range v.p {
for _, rel := range relations {
var subject string

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 1, 2017

Member

This should be query or something, subject implies that it's a policy subject which isn't the case

This comment has been minimized.

Copy link
@zikes

zikes Nov 1, 2017

Author Contributor

Agreed. I struggled to find the right variable name for this, and query still doesn't seem quite right, but it is at least less confusing than subject.


type Database struct {
Migrations *migrate.MemoryMigrationSource
CreatePolicy string

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 1, 2017

Member

Please rename this to QueryInsertPolicy

CreatePolicyResources string
CreatePolicyResourcesRel string
CreatePolicySubjects string
CreatePolicySubjectsRel string

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 1, 2017

Member

And subsequently for all of the above


import migrate "github.com/rubenv/sql-migrate"

type Database struct {

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 1, 2017

Member

Please rename this to sqlStatements - exporting it is not required because it's an internal struct

This comment has been minimized.

Copy link
@zikes

zikes Nov 1, 2017

Author Contributor

In order to support external additions of databases which are not officially supported, this would need to remain exported. In that case, would you prefer SQLStatements?

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 1, 2017

Member

Yes, I think Statements should actually be enough!

db *sqlx.DB
schema []string
db *sqlx.DB
schema []string

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 1, 2017

Member

Doesn't seem like this is being used any more

This comment has been minimized.

Copy link
@zikes

zikes Nov 1, 2017

Author Contributor

Agreed. Removing this might require changing the signature of the NewSQLManager function, currently func NewSQLManager(db *sqlx.DB, schema []string) *SQLManager. I could change that to func NewSQLManager(db *sql.DB) *SQLManager or continue accepting schema and ignore it until a major version release?

@aeneasr

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

Let me know when this is ready for another pass!

@zikes

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

I believe I've implemented all your requests, please review and merge at your convenience.

@aeneasr
Copy link
Member

left a comment

Thank you, a couple more questions and also some changes I'd like to see

},
},
},
QueryInsertPolicy: `INSERT INTO ladon_policy(id, description, effect, conditions) SELECT $1::varchar, $2, $3, $4 WHERE NOT EXISTS (SELECT 1 FROM ladon_policy WHERE id = $1)`,

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 5, 2017

Member

Do you know why this (::varchar) was previously not necessary?

This comment has been minimized.

Copy link
@zikes

zikes Nov 7, 2017

Author Contributor

The PostgreSQL driver's type inference was returning two different types for the first and second instances of $1 in the prepared statement, so the ::varchar settled that. It was originally unnecessary because $1 wasn't reused later in this query, the ID value was passed in twice.

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 10, 2017

Member

Thank you!

Migrations: []*migrate.Migration{
{
Id: "1",
Up: []string{

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 5, 2017

Member

I think it makes sense to use the same up/down statements wherever possible which in this case would be that postgres and mysql have the same up/down statements for id 1. What do you think?

This comment has been minimized.

Copy link
@zikes

zikes Nov 7, 2017

Author Contributor

When I was reorganizing with the goal of adding official support for more databases, it made more sense to me to have each database's statements remain separate for readability. I can revert this since the library will only be supporting MySQL & PostgreSQL.

return errors.Errorf("Database %s is not supported", s.database)
}

if _, err = tx.Exec(Databases[s.database].QueryInsertPolicy, policy.GetID(), policy.GetDescription(), policy.GetEffect(), conditions); err != nil {

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 5, 2017

Member

By using db.Rebind() we can actually further streamline the SQL statements and use ? everywhere instead of dialect-specific parameters.

break
default:
return errors.Errorf("Database driver %s is not supported", s.db.DriverName())
if _, err := tx.Exec(query, id, template, compiled.String(), strings.Index(template, string(policy.GetStartDelimiter())) >= -1); err != nil {

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 5, 2017

Member

By using db.Rebind() we can actually further streamline the SQL statements and use ? everywhere instead of dialect-specific parameters.

if _, err := tx.Exec(query, id, template, compiled.String(), strings.Index(template, string(policy.GetStartDelimiter())) >= -1); err != nil {
return errors.WithStack(err)
}
if _, err := tx.Exec(queryRel, policy.GetID(), id); err != nil {

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 5, 2017

Member

By using db.Rebind() we can actually further streamline the SQL statements and use ? everywhere instead of dialect-specific parameters.

default:
return nil, errors.Errorf("Database driver %s is not supported", s.db.DriverName())
}
query := Databases[s.database].FindRequestCandidates

rows, err := s.db.Query(query, r.Subject, r.Subject)

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 5, 2017

Member

I missed db.Rebind here myself but I think we should add it here too.

@zikes

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2017

This PR is ready for an additional review.

@aeneasr
Copy link
Member

left a comment

Awesome, thank you so much for staying with this PR and addressing all the issues. There are only two more, very minor, renames left. Once they are done I'll merge it as it LGTM!

},
}

var Databases = map[string]Statements{

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 8, 2017

Member

Rename to Migrations

QueryInsertPolicyResourcesRel string
QueryInsertPolicySubjects string
QueryInsertPolicySubjectsRel string
FindRequestCandidates string

This comment has been minimized.

Copy link
@aeneasr

aeneasr Nov 8, 2017

Member

Rename to QueryRequestCandidates

@kminehart

This comment has been minimized.

Copy link

commented Nov 8, 2017

Thanks @arekkas. I've made the 2 changes that were requested :)

This will close #86, as CRDB support will be provided via. an external library.

@andreis

This comment has been minimized.

Copy link

commented Nov 8, 2017

We actually need cockroachDB support, so many thanks @kminehart and @arekkas and let me know if there's any way I can help!

@kminehart

This comment has been minimized.

Copy link

commented Nov 8, 2017

@andreis Thank @zikes ! I just changed a few variable names for him.

@aeneasr

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

Thank you for all your hard work, sorry for the long response times, the past days have been very intense!

@aeneasr aeneasr merged commit 5892158 into ory:master Nov 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 88.565%
Details
@aeneasr

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

By the way, if you supply CockroachDB support in a separate repo I would be more than happy to link it in the README

@zikes

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2017

Thanks! The CRDB repo is at https://github.com/wehco/ladon-crdb

@aeneasr

This comment has been minimized.

Copy link
Member

commented Nov 12, 2017

7285d8c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.