Skip to content

SQLite filename will depend on schema definition strings#75

Merged
davidstrauss merged 2 commits intolcache:masterfrom
ndobromirov:l1-sqlite-schema-changes
Dec 17, 2016
Merged

SQLite filename will depend on schema definition strings#75
davidstrauss merged 2 commits intolcache:masterfrom
ndobromirov:l1-sqlite-schema-changes

Conversation

@ndobromirov
Copy link
Copy Markdown
Contributor

@ndobromirov ndobromirov commented Dec 12, 2016

Overview

This pull request:

  • Implements changes described in issue Manage schema updates in SQLiteL1 #66
  • Adds a feature - SQLite file-name to depend on schema, so schema changes will reflect in new schema to be created from scratch.

Summary

There is a new utility that holds all the SQL queries needed to create the schema.
Based on them a hash is generated and added to the file name, making it unique.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 12, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 3bcb55e on ndobromirov:l1-sqlite-schema-changes into 85ca4fd on lcache:master.

Copy link
Copy Markdown
Member

@davidstrauss davidstrauss left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but let's change the hash used.

src/SQLiteL1.php Outdated
protected static function getDatabaseHandle($pool)
{
$path = join(DIRECTORY_SEPARATOR, array(sys_get_temp_dir(), 'lcache-' . $pool));
$schemaId = md5(implode(';', self::schemaStatements()));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While we hardly need to worry about collision issues here, government projects freak out if you even call MD5 from your code. They then require you to submit a statement for each use any why it's not cryptographically vulnerable. So, I recommend a truncated SHA-512 or a hash that doesn't even pretend to be cryptographically sound.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will go with the sha512, even though it's an overkill.

Off-topic: Shouldn’t we sanitize the $pool value in any way?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The pool should only be set based on local system configuration; I don't think it needs to be sanitized.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 13, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ce0a4d0 on ndobromirov:l1-sqlite-schema-changes into 85ca4fd on lcache:master.

@davidstrauss davidstrauss merged commit a11946d into lcache:master Dec 17, 2016
ndobromirov added a commit to ndobromirov/lcache that referenced this pull request Dec 27, 2016
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.

3 participants