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

Bugfix rollup: Fix issue with extra characters in generated SQL; Fix ToAliasReference for already referenced values; Fix Alias/Reference for Maybe Entity #191

Merged
merged 9 commits into from
Aug 30, 2020

Conversation

belevy
Copy link
Collaborator

@belevy belevy commented Jul 6, 2020

in_ was generating invalid mysql statements by double wrapping value lists. This existed before 3.3.3.1. Also fixed an issue with the generated value names in subqueries getting "`"s erroneously added for each nesting

Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@belevy
Copy link
Collaborator Author

belevy commented Jul 6, 2020

@parsonsmatt the code that triggered this bug was

posts ^. FeedPostPermissionId `in_` valList [6..8]

unsafeSqlFunction isn't wrapping in parens at all but unsafeSqlBinOp was. Not sure if we should just Never add parens or if this safer check is better. Either way, bumped and were gonna be relying on this commit at work.

@belevy
Copy link
Collaborator Author

belevy commented Jul 7, 2020

Found another bug while working on this query. Was doing a SubQuery join on a Union and got errors where the union alias (u) was being referenced rather than the subquery (q) of the union. This led to mysql throwing an error u.v_id is not a field. Figured since this PR was still open, better not to create extra versions.

@belevy belevy changed the title Fix issue with extra characters in generated SQL Fix issue with extra characters in generated SQL; Fix ToAliasReference for already referenced values Jul 7, 2020
@belevy belevy changed the title Fix issue with extra characters in generated SQL; Fix ToAliasReference for already referenced values Bugfix rollup: Fix issue with extra characters in generated SQL; Fix ToAliasReference for already referenced values; Fix Alias/Reference for Maybe Entity Jul 10, 2020
@@ -767,6 +771,7 @@ instance ( ToAlias a
type family ToAliasReferenceT a where
ToAliasReferenceT (SqlExpr (Value a)) = SqlExpr (Value a)
ToAliasReferenceT (SqlExpr (Entity a)) = SqlExpr (Entity a)
ToAliasReferenceT (SqlExpr (Maybe (Entity a))) = SqlExpr (Maybe (Entity a))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very unrelated question, but why do both ToAliasT and ToAliasReferenceT exist? They seem to do the same thing (?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They might be able to be merged but it's not clear to me that they will always be the same. They initially started as associated type families in their respective classes. Semantically they are different though, an alias is something like "value as alias" where an alias reference is like "q.v".

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Asked a question re the ticks

Looks great! Mind getting the conflicts resolved?

src/Database/Esqueleto/Internal/Internal.hs Outdated Show resolved Hide resolved
Comment on lines +2253 to +2261
let
valueToFunctionArgParens v =
case v of
ERaw p f -> first (parensM p) (f info)
EAliasedValue i _ -> aliasedValueIdentToRawSql i info
EValueReference i i' -> valueReferenceToRawSql i i' info
ECompositeKey _ -> throw (CompositeKeyErr SqlFunctionError)
(argsTLB, argsVals) =
uncommas' $ map valueToFunctionArgParens $ toArgList arg
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix #201

@belevy
Copy link
Collaborator Author

belevy commented Aug 27, 2020

@parsonsmatt I fixed #201 while while resolving the conflicts. Seemed better to fix it in one PR and its not much more code. See the commented code for the fix. Additionally removed the other ERaw lambda that was left but it uses the same code that unsafeSqlFunction uses

changelog.md Outdated Show resolved Hide resolved
esqueleto.cabal Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
@parsonsmatt parsonsmatt merged commit f9a8088 into bitemyapp:master Aug 30, 2020
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.

None yet

3 participants