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.

Sign up for GitHub

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

Jump to bottom

Composable sql interpolation #6718

Merged
merged 9 commits into from
Jan 4, 2023
Merged

Composable sql interpolation #6718

merged 9 commits into from
Jan 4, 2023

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Jan 3, 2023 •
edited by fm3
Loading

  • Adds a new string interpolation-based method for constructing composable SQL queries, where all values are sent to the database separate from the query text, which greatly reduces the risk of SQL injection attack surface
  • Move utils.SQLHelpers to utils.sql.SQLHelpers

Steps to test:

  • Run a voxelytics workflow and check the workflow report in wk
  • Also, there are some unit tests that verify the inner workings of the sql interpolation
  • Ready for review

@normanrz normanrz requested a review from fm3 January 3, 2023 09:37
fm3
fm3 reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

This is good stuff, I’m looking forward to using it everywhere!
I added a few minor comments.

I don’t think we can make this all that much cleaner, you already got pretty far there.

On naming the string modifier – how about just q for query? I think we would want to use this for all queries, whether we will use the nesting feature or not – then the n is no longer so relevant for the usages. What do you think? sql would also be nice, but that might cause confusion with the existing one provided by slick.

app/utils/SQL.scala Outdated Show resolved Hide resolved
app/utils/SQL.scala Outdated Show resolved Hide resolved
app/utils/SQL.scala Outdated Show resolved Hide resolved
app/utils/SQL.scala Outdated Show resolved Hide resolved
app/utils/SQL.scala Outdated Show resolved Hide resolved
app/utils/SQL.scala Outdated Show resolved Hide resolved
@normanrz
Copy link
Member Author

normanrz commented Jan 3, 2023

On naming the string modifier – how about just q for query?

I like q.

How would you structure the files/packages? Should this live under utils or utils.sql or something?

@fm3
Copy link
Member

fm3 commented Jan 3, 2023 •
edited
Loading

utils.sql sounds good. This could also hold the existing SQLHelpers file (which should probably be split into a few files, but that can wait for a later PR)

@normanrz normanrz self-assigned this Jan 3, 2023
@normanrz normanrz marked this pull request as ready for review January 3, 2023 14:32
@normanrz normanrz changed the title Nested sql interpolation Composable sql interpolation Jan 3, 2023
@normanrz normanrz requested a review from fm3 January 3, 2023 14:38
fm3
fm3 reviewed Jan 4, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

I added another commit following some code style recommendations from intellij, nothing to worry about.
I also added two more minor comments:

app/utils/sql/SqlInterpolation.scala Outdated Show resolved Hide resolved
test/backend/SQLTestSuite.scala Outdated Show resolved Hide resolved
fm3
fm3 approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Awesome 🎉

I think we should take some time soon to propagate this to all the DAOs. In the process, we will likely notice more features missing here, but they can then be added as needed.

@fm3 fm3 mentioned this pull request Jan 4, 2023
Closed
@normanrz normanrz merged commit 50de231 into master Jan 4, 2023
@normanrz normanrz deleted the nested-sql branch January 4, 2023 15:04
hotzenklotz added a commit that referenced this pull request Jan 5, 2023
@hotzenklotz
Merge branch 'master' of github.com:scalableminds/webknossos into pri…
f1e34ac 
…cing

* 'master' of github.com:scalableminds/webknossos:
  Replace babel with esbuild for tests and for webpack (#6527)
  Use less ram when initializing data textures (#6711)
  Composable sql interpolation (#6718)
  Release 23.01.0 (#6717)
  add license checker (#6715)
  Fix brush performance for coarse mags & avoid some unnecessary re-renders (#6708)
  Change backend format command (#6697)
  Improve layout of terms of services check (#6712)
@fm3 fm3 mentioned this pull request Jan 9, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fm3 fm3 fm3 approved these changes

Assignees

@normanrz normanrz

Labels
backend refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@normanrz @fm3