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

Update database schema to include unique keys #149

Merged
merged 3 commits into from
Jun 6, 2013
Merged

Update database schema to include unique keys #149

merged 3 commits into from
Jun 6, 2013

Conversation

cameronk
Copy link

@cameronk cameronk commented Jun 3, 2013

Slightly updated the basic db schema setup to include unique columns;
this required some columns to be converted to the 'varchar' type. This presents some security benefits (i.e. queries would fail should a required column, such as 'access_token', not be present), and allows inline editing features in phpMyAdmin.

Slightly updated the basic db schema setup to include unique columns;
this required some columns to be converted to the 'varchar' type.
@bshaffer
Copy link
Owner

bshaffer commented Jun 4, 2013

What DBMS's has this been tested with, other than MySQL?

@dsquier
Copy link
Contributor

dsquier commented Jun 4, 2013

A few comments:

  1. I agree that there should be unique keys on certain columns. I've done this for my implementation, but it adds dependencies for handling a primary key violation, which should be explored further.

  2. I'm not sure constraining all keys to 255-characters is the way to go. For columns such as access_token, refresh_token, and authorization_code, I'd prefer to match the column width to the token/code length generated by the library (i.e., 40 characters).

  3. I'm not sure what value backticks provide on table and column names. In fact, I think they're dangerous to use in any DDL (similar to enclosing tables or columns with single tick or quotation) as some DBMSs will create the object literally and if a subsequent select against the column doesn't match the literal, you'll get an error. Here is an example from Oracle:

SQLPLUS>create table ab ("a" number, "B" number);

Table created.


SQLPLUS>insert into ab ("a", "B") VALUES (1,2);

1 row created.


SQLPLUS>desc ab;
 Name                  Null?    Type
 --------------------- -------- ------------
 a                              NUMBER
 B                              NUMBER

SQLPLUS>select a from ab;
select a from ab
       *
ERROR at line 1:
ORA-00904: "A": invalid identifier


SQLPLUS>select "a" from ab;

         a
----------
         1

4) Finally, I'd like to see any changes to the DDL move towards ANSI SQL standards. This would allow portability to other RDBMSs, such as Oracle, PostgreSQL, etc. I believe the data model is simple enough to conform to, say, ANSI SQL-92 without loosing any functionality.

@dsquier
Copy link
Contributor

dsquier commented Jun 4, 2013

FWIW, the DDL I am currently using is available at:

https://github.com/dsquier/oauth2-server-php-mysql

There are some areas for improvement (i.e., conforming to ANSI SQL and a some refinement of scope storage), but it's working with the current development branch on the library without issues.

Also added another header field for DBMS's which the schema has been
tested with.
@cameronk
Copy link
Author

cameronk commented Jun 4, 2013

After some more testing, it seems that this doesn't work with Oracle, Postgre. or MS SQL Server. As @dsquier said, it's probably a good idea to conform with a standard such as ANSI SQL-92 for portability. However, I'm not particularly familiar with the syntax of DBMS's other than MySQL. I've pushed another commit, updating the README to include a schema which is compliant with MySQL and SQLite for the time being.

@dsquier
Copy link
Contributor

dsquier commented Jun 5, 2013

I wasn't correct in #4 bringing ANSI SQL into the picture. I should have left this out, but let me clarify.

Since this is all table (and index) definitions, I doubt we'll be able to get something to work across more than one database, much less multiple ones. More importantly, there is no ANSI standard for DDL granular enough to define tables cross-platform. For example, Oracle wants VARCHAR2 instead of VARCHAR, while MySQL wants the opposite.

Separated the schema into two sections - after reading up on some
documentation I discovered that MySQL + SQLite accept both their own
CONSTRAINT syntax as well as that of Postgre and MS SQL. However Oracle
continues to be stubborn and only accept VARCHAR2 types. It also doesn't
recognize the "text" type - because of this, I've converted all text
fields to VARCHAR/VARCHAR2 fields with a default length of 255 chars
(apart from password, which is set to 2000 to accommodate hashing
algorithms).
@cameronk
Copy link
Author

cameronk commented Jun 6, 2013

The above commit addresses all major DBMS's (MySQL, SQLite, Oracle, MS SQL, PostgreSQL) and contains the correct schema for each system.

bshaffer added a commit that referenced this pull request Jun 6, 2013
Update database schema to include unique keys
@bshaffer bshaffer merged commit 30679a6 into bshaffer:develop Jun 6, 2013
@bshaffer
Copy link
Owner

bshaffer commented Jun 6, 2013

@dsquier awesome repo! I can call this out in the README if you'd like.

@dsquier
Copy link
Contributor

dsquier commented Jun 6, 2013

@bshaffer You are more than welcome to include, I'd be honored!

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.

3 participants