[ovirt-devel] Writing SQL queries in DAO code

Eli Mesika emesika at redhat.com
Mon Mar 27 09:45:16 UTC 2017 

On Mon, Mar 27, 2017 at 12:26 PM, Yevgeny Zaspitsky <yzaspits at redhat.com>
wrote:

> > I don't think that looking in SQL in Java code is ​clear than looking in
> a SP code
>
> Looking in SQL isn't the problem I'm trying to solve, but finding that.
>
> > 1) You will pass more data on the wire instead of calling a SP with
> parameters
>
> Passing the SQL string shouldn't be a problem unless that is very long one
> (several K) and then we probably do something wrong.
>
> > 2) Your data that is passed on the wire is exposed to attacks since you
> will have to implement DB security in the engine level (for example hidden
> columns).
>
> IIUC currently querying tables/views with hidden columns is implemented in
> a SP that consist of at least 2 SQL's, so those aren't good candidates for
> my proposal and will stay as is.
> BTW how other projects resolve the security problem? AFAIK usually hidden
> columns are hidden by defining a proper view that do not include those.
>

​That's a bad solution as long as your data is passed unmasked on the wire ​
Database security should be done in the database level and you will not be
able to do that without using SPs



>
> > 3) Changes in the SQL code done in patches may be more complicated to
> track
>
> Most of SQL code changes involve changes in a DAO too, so this shouldn't
> be an issue.
>
​It is !!! , changes in DAO are easy to track since it is a Java code , you
are suggesting to write the SQL itself ​


>
> > 4) SQL Injection
>
> Again, how other projects resolve the security problem? Internet is full
> of articles/blogs of why stored procedures should be avoided ("avoid
> ​​
> stored procedures" Google query returned ~6.6M results), so I guess there
> are some other approaches for the security issue if that exists.
>

​"use stored procedures" Google query returned About 4,840,000 results ​



>
> > 5) I think that SP performs better than SQL inside Java
>
> Do we have a measurement of how much better?
> One of my intentions for this proposal is that people evidently avoid
> creating neat SQL queries and re-use the existing ones, which has much
> bigger performance impact. I guess that the biggest limit here is how
> complicated that procedure is in the engine code.
>

​That's why we have code review process and we should nack such ​patches ,
so you think that if people are not familiar with Java8 syntax for example
we should move this code to be performed by a "easier" mechanism ?
If people are not doing the right thing , we have gerrit for that, we can
comment , nack , whatever to make the code good



>
>
> On Mon, Mar 27, 2017 at 11:09 AM, Eli Mesika <emesika at redhat.com> wrote:
>
>>
>>
>> On Mon, Mar 27, 2017 at 1:56 AM, Moti Asayag <masayag at redhat.com> wrote:
>>
>>>
>>>
>>> On Sun, Mar 26, 2017 at 5:12 PM, Yevgeny Zaspitsky <yzaspits at redhat.com>
>>> wrote:
>>>
>>>> Hi All,
>>>>
>>>> Recently I had a task of performance improvement in one of our network
>>>> related flows and had some hard time following our DAL code and one of the
>>>> outcomes of the task was defining a couple of new quite simple, but neat
>>>> queries.
>>>> When I came to coding those new queries I remembered how hard was
>>>> following the existing DAL code, I decided to make my own new methods
>>>> clearer. So I created [1] and [2] patches.
>>>>
>>>> Everything is quite standard there beside the fact that they do not use
>>>> any stored procedure, but use SQL directly, IMHO by that they save time
>>>> that I spent in trying to follow what a DAO method does. Looking into the
>>>> method code you get the understanding of what this method is all about:
>>>>
>>>>    - no looking for a stored procedure name that is buried in the DAO
>>>>    class hierarchy
>>>>    - no looking for the SP definition
>>>>
>>>>
>>> There are additional pros and cons for the suggestion to consider:
>>>
>>> Pros:
>>>
>>>    1. No need to run engine-setup after changing DB related code (in
>>>    case of SQL inside Java).
>>>
>>> Cons:
>>>
>>>    1. DAO files might become very long.
>>>    2. If you decide to return the business entity associated with the
>>>    DAO as a returned object, you won't know as a caller which fields to expect
>>>    to be populated, which lead to 3:
>>>    3. An inflation of business entities to represent partial populated
>>>    business entity or inflation of mappers inflation (this will be required
>>>    for SP as well).
>>>    4. SQL code inside of Java:
>>>    1. Beside of the fact that a multi-line concatenated string that
>>>       cannot be easily copied and run with psql, it means that we should compile
>>>       the code in order to test the change (vs building with DEV_REBUILD=0 which
>>>       only package the SQL file).
>>>       2. No syntax highlighting when performing code review. i.e. I
>>>       don't think reviewing a patch such as
>>>       https://gerrit.ovirt.org/#/c/66729/10/packaging/dbscripts/ne
>>>       twork_sp.sql
>>>       <https://gerrit.ovirt.org/#/c/66729/10/packaging/dbscripts/network_sp.sql>
>>>       would be more clear inside a java file.
>>>       3. The user permissions management is implemented on DB level.
>>>       That means that SQL will be more complex (more concatenated java lines).
>>>    5. Stored procedure are cached by project's code. See
>>>    SimpleJdbcCallsHandler.getCall(), while the
>>>    NamedParameterJdbcTemplate are cached by spring's code which seems less
>>>    optimal (sync all calls using synchronized vs using ConcurrentHashMap as in
>>>    SP cache).
>>>    6. With the NamedParametersJdbcTemplate there is no use of the
>>>    DbEngineDialect. What's the impact of it ?
>>>
>>> So besides 5 and 6, the rest is a matter of style. I'd like to hear more
>>> opinions from other members.
>>>
>>
>> ​I agree with all you wrote Moti
>> I don't think that looking in SQL in Java code is ​clear than looking in
>> a SP code
>> Additionally
>> 1) You will pass more data on the wire instead of calling a SP with
>> parameters
>> 2) Your data that is passed on the wire is exposed to attacks since you
>> will have to implement DB security in the engine level (for example hidden
>> columns)
>> 3) Changes in the SQL code done in patches may be more complicated to
>> track
>> 4) SQL Injection
>> 5) I think that SP performs better than SQL inside Java
>>
>> I see no real reason to replace the SPs with SQL code , SP is just a
>> container for SQL code
>>
>>
>>
>>>
>>> Regards,
>>> Moti
>>>
>>> So I'd like to propose moving towards this approach in general in all
>>>> cases when nothing beyond a simple SQL is needed (no stored procedure
>>>> programming language is needed).
>>>> From my experience with the performance improvement task it looks like
>>>> people avoid adding new queries for a specific need of a flow, instead they
>>>> use the existing general ones (e.g. dao.getAllForX()) and do the actual
>>>> join in the bll code.
>>>> IMHO the proposed approach would simplify adding new specific queries
>>>> and by that would prevent a decent part of performance issues in the future.
>>>>
>>>> I do not propose changing all existing SP's to inline queries in a
>>>> once, but to allow adding new queries this way. Also we might consider
>>>> converting relatively simple SP's to inline SQL statements later in a
>>>> graduate way.
>>>>
>>>> [1] - https://gerrit.ovirt.org/#/c/74456
>>>> [2] - https://gerrit.ovirt.org/#/c/74458
>>>>
>>>> Regards,
>>>> Yevgeny
>>>>
>>>> _______________________________________________
>>>> Devel mailing list
>>>> Devel at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>
>>>
>>>
>>>
>>> --
>>> Regards,
>>> Moti
>>>
>>> _______________________________________________
>>> Devel mailing list
>>> Devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170327/0b606a7a/attachment-0001.html>

More information about the Devel mailing list