-
Notifications
You must be signed in to change notification settings - Fork 27
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
Ignite row count #1215
Ignite row count #1215
Conversation
✅ Deploy Preview for papaya-valkyrie-395400 canceled.
|
val query = new SqlFieldsQuery(s"select COUNT(1) from ChildOrder$whereClause") | ||
val cursor = childOrderCache.query(query) | ||
|
||
val countValue = cursor.getAll().get(0).get(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we need to put a guard around this? I guess not as there should always be a count (even if its zero)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and we need generally error handling in the whole on the ignite order store + ignite order data provider.
Started and parked the refactoring because multiple PR is touching this file.
Created dedicated issue #1216
//todo should this be COUNT_BIG? | ||
val whereClause = if(sqlFilterQueries == null || sqlFilterQueries.isEmpty) "" else s" where $sqlFilterQueries" | ||
val query = new SqlFieldsQuery(s"select COUNT(1) from ChildOrder$whereClause") | ||
val cursor = childOrderCache.query(query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we should think about is whether we'd want to parametrize the sql statement, at the moment I think we'd be vulnerable to a sql injection attack on the where clause. Obv this is not a problem for the demo, but something we might want to clarify from a usage perspective.
Would be interested in @rumakt 's views on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the ignite side?
On the vuu side the sql filter query is generated from our ui filter query syntax.
Do you mean the interface for this method should not accept a string filter query or worried about attacker sending their own VUU sql syntax via the websocket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be worried about an attacker submitting something like this:
columnName ='FOO';DELETE * FROM Orders' in a filter statement and it making its way all the way to Ignite.
I'm not sure if Ignite supports prepared statements:
"Select * from Orders Where column = ?"
And we get the driver/client lib to do the software parse, (so that a sql attack can't happen) then we do like this:
query.setParameter(0, );
If that makes sense. Its not a problem for the demo code, but might imply the string concat is ok in prod system.
No description provided.