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

More validations for code that generates invalid SQL #18

Open
wants to merge 3 commits into
base: developer
Choose a base branch
from

Conversation

lucasnodari
Copy link
Contributor

This adds many validations for constructions that could generate invalid SQL.

Empty table names are not accepted in mysql. This is also useful when using a name from a variable, because the variable could be null or not set, so it reports this error as soon as it is introduced.

$q->table()->select()->execute();
// before: select * from `` -- SQL error Incorrect table name ''
// now: throws Exception: Table name cannot be empty.

Refactored subquery generation by closure and added validation functions for generated subqueries.

$q->table('tbl_a')->select()->join('tbl_b', function($q){})->execute();
// before: select * from `tbl_a` left join `tbl_b` on ``  `` -- syntax error
// now: throws Exception: Subquery generator did not generate a valid subquery.

$q->table(['tbl_a' => function($q){}])->select()->execute();
// before: select * from (select * from ``) as `tbl_a` -- SQL error Incorrect table name ''
// now: throws Exception: Subquery generator did not generate a valid subquery.

$q->table('tbl')->select()->where(function($q){ $q->table('error');})->execute();
// before: select * from `tbl` where (  ) -- syntax error
// now: throws Exception: Subquery generator did not generate a valid subquery.

Added validation for missing last two join parameters, when not using second parameter as closure, Expression or SelectJoin.

$q->table('tbl_a')->select()->join('tbl_b','id')->execute();
// before: select * from `tbl_a` left join `tbl_b` on `1`  ``
// now: throws Exception: When using non nested join conditions, an operator and a reference key is required.

@lucasnodari
Copy link
Contributor Author

I use this library a lot in a project, in my fork I have also made many other changes and implemented some new features, maybe you have seen it already, if not, then please check it out.

Some changes require at least PHP 7.0, I see that you want to keep this library compatible with some PHP 5 versions. These changes are mostly scalar type hints, they could easily be removed for compatibility.

@mario-deluna
Copy link
Member

@lucasnodari Your fork looks very promising, Im looking forward to start a v2 branch with some major refactorings.

I see that you want to keep this library compatible with some PHP 5 versions.

Exactly, thats also why im hesitating with adding more features to the current version of Hydrahon.

For V2 it would be absurd to not require at least PHP7, so you adding type hints is great!

My current plan would be to base v2 on your fork and start fixing some structural issues that have been bothering me for a long time. For example the static grammar binding or the way the execution callback is handled. I really want to keep the API almost identical to V2 to make the migration painless.

Nonetheless I believe some things have to go, please let me know if some of the following things would break your implementations:

  • Removal of query macros and instead rely on query subclasses.
  • Different and private internal data structures. If you directly access things like the joins array this might be a dealbreaker.

@mario-deluna mario-deluna added this to Backlog in 2.0 Mar 19, 2019
@lucasnodari
Copy link
Contributor Author

@lucasnodari Your fork looks very promising, Im looking forward to start a v2 branch with some major refactorings.

Thanks!

Removal of query macros and instead rely on query subclasses.

I've never used macros, I have used once the builder flags for some small tests, but it is nothing important.

Different and private internal data structures. If you directly access things like the joins array this might be a dealbreaker.

Never did this, I think.

Exactly, thats also why im hesitating with adding more features to the current version of Hydrahon.

Ok, maybe we should close this PR then, maybe add into v1 some simple features from the v2 branch like variadics, or a simplified implementation of unions.

My current plan would be to base v2 on your fork and start fixing some structural issues that have been bothering me for a long time. For example the static grammar binding or the way the execution callback is handled. I really want to keep the API almost identical to V2 to make the migration painless.

Some currently open issues are now solved in v2, issue #11 was fixed by making some small changes in the joins translation, issue #15 is more or less solved because support for union statements was implemented in my fork.

The current support for unions is documented on the readme.md, it may still need some adjustments, because the way it works now, the queries in unions are generated as a subqueries to the base query, this results in some problems like, it is not possible to add unions "recursively", because it would result in an union keyword inside a subquery, and that is not allowed syntax in mysql.

Another issue on unions is the scope of attached clauses, clauses attached to the subquery will always be local, clauses attached to the base query, will always be global, this is not really a problem in the current implementation, because the resulting query makes it very clear that this is the case.

I've also added another documentation file for other features, it is named Extra_Features. It currently explains how to use the "secret" direct object feature that is already possible in v1, the variadic select, the new alternative to using raw or Expression for inserting nulls, multiple tables in from clauses and a way to force an operand to be interpreted as an identifier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2.0
  
Backlog
Development

Successfully merging this pull request may close these issues.

None yet

2 participants