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

Use new sql interpolator in more DAOs, clean up #6723

Merged
merged 15 commits into from
Jan 12, 2023
Merged

Use new sql interpolator in more DAOs, clean up #6723

merged 15 commits into from
Jan 12, 2023

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jan 5, 2023

  • propagates the use of the new sql interpolator with q"myQuery" that was introduced in Composable sql interpolation #6718 to more DAOs
  • also adapts the types of the access queries (meaning the DAOs that are NOT yet adapted must re-stringify the access queries using .debugInfo
  • splits classes of the file SQLHelpers into individual files
  • adds support for enums, boundingboxes, vec3ints, arrays in sql interpolator

URL of deployed dev instance (used for testing):

Issues

TODO

  • split SQLHelpers file, one file per class
  • make access queries sqlTokens
  • solve setting custom enumerations
  • fix setting varchar[] (e.g. annotation tags)
  • fix setting nulled jsonb
  • fix setting Vec3Double
  • check that error logging does not leak user data

Follow-up

  • adapt all remaining queries
  • remove then-unused escaping/unescaping code
  • remove then-unused SqlTypeImplicits trait
  • check that no usages of #$ remain
  • check that no usages of debugInfo remain
  • check that no usages of sql"" and sqlu"" remain, remove their import
  • replace the Any/match approach to an implicit conversion approach to re-enable type checks at compile time

Steps to test:

  • Test creating/importing/updating/deleting datasets, annotations, private links, folders
  • Test with multi-user setup that access checks still work (e.g. dataset list, with folder permissions)
  • CI helps as well
  • do some monkey-testing of things that I am not thinking of 😅

  • Ready for review

@fm3 fm3 self-assigned this Jan 5, 2023
@fm3 fm3 changed the title [WIP] use new sql interpolator everywhere Use new sql interpolator in more DAOs, clean up Jan 9, 2023
@fm3 fm3 marked this pull request as ready for review January 9, 2023 15:16
@fm3 fm3 requested review from normanrz and jstriebel January 9, 2023 15:23
@fm3
Copy link
Member Author

fm3 commented Jan 9, 2023

@normanrz Could you have a look at the changes in the SqlInterpolation file, and possibly check a few usages to see if I use this the way you intended it? @jstriebel agreed to look at the other changes :) It would also be appreciated if you could do some random testing. The fact that all SQL errors are logged and reported to slack makes me optimistic that we should find errors quickly

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Looks good in general. A few notes:

  • For INSERTs, you could use SqlToken.tuple(...) to supply the values instead of interpolating them separately into the query
  • Not sure whether boolean constants should be written in or passed in
  • Please add tests for each new datatype that you added to SqlValue
  • We should think of more helper functions to make inserts, updates and upserts less verbose
  • Since you migrated all the big DAOs, it should be quick to do the rest, no?

app/utils/sql/SecuredSQLDAO.scala Show resolved Hide resolved
app/models/annotation/Annotation.scala Show resolved Hide resolved
app/models/annotation/Annotation.scala Show resolved Hide resolved
app/models/binary/DataSet.scala Outdated Show resolved Hide resolved
app/models/voxelytics/VoxelyticsDAO.scala Outdated Show resolved Hide resolved
conf/application.conf Outdated Show resolved Hide resolved
app/utils/sql/SQLDAO.scala Outdated Show resolved Hide resolved
@fm3
Copy link
Member Author

fm3 commented Jan 10, 2023

Thanks for your feedback!

  • I changed the boolean constants to pass values
  • I added tests
  • I hope to adapt the remaining DAOs in the coming days, but I think a separate PR for that would make sense. There are still more than 100 queries to be adapted.
  • I created follow-up issue SQL helper functions for updates, inserts, upserts #6730 for the helper functions, as it is not immediately obvious to me how to design them

@normanrz normanrz mentioned this pull request Jan 10, 2023
2 tasks
@fm3 fm3 mentioned this pull request Jan 11, 2023
1 task
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

LGTM, all things that looked weird are already part of the follow-up points 👍 🚀
Props for adding tests! ❤️

@fm3 fm3 merged commit 77f77e3 into master Jan 12, 2023
@fm3 fm3 deleted the sql-refactoring branch January 12, 2023 07:48
@fm3 fm3 mentioned this pull request Jan 12, 2023
1 task
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