<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 27, 2017 at 5:30 PM, Yevgeny Zaspitsky <span dir="ltr"><<a href="mailto:yzaspits@redhat.com" target="_blank">yzaspits@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="gmail-">On Mon, Mar 27, 2017 at 12:45 PM, Eli Mesika <span dir="ltr"><<a href="mailto:emesika@redhat.com" target="_blank">emesika@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-size:large"><br></div><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Mar 27, 2017 at 12:26 PM, Yevgeny Zaspitsky <span dir="ltr"><<a href="mailto:yzaspits@redhat.com" target="_blank">yzaspits@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div><span class="gmail-m_4993947824586612435m_7759201291858962337gmail-"><div>> I don't think that looking in SQL in Java code is clear than looking in a SP code<br><br></div></span>Looking in SQL isn't the problem I'm trying to solve, but finding that.<span class="gmail-m_4993947824586612435m_7759201291858962337gmail-"><br><br>> 1) You will pass more data on the wire instead of calling a SP with parameters<br></span></div><br>Passing the SQL string shouldn't be a problem unless that is very long one (several K) and then we probably do something wrong.<br><br>> 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).<br><br></div><div>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.<br></div>BTW how other projects resolve the security problem? AFAIK usually hidden columns are hidden by defining a proper view that do not include those.<span class="gmail-m_4993947824586612435m_7759201291858962337gmail-"><br></span></div></div></blockquote></span><div><br><div>That's a bad solution as long as your data is passed unmasked on the wire <br></div></div></div></div></div></blockquote></span><div><br>In some cases we do send a sensitive info in the current solution that uses SP's.<br></div><div>Should we use (maybe already) a kind of secured connection to DB? <br><br></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div></div><div>Database security should be done in the database level and you will not be able to do that without using SPs<br></div></div></div></div></div></blockquote></span><div><br>I'm not a DB security expert, but from my experience from my previous jobs none of them used "SP's only" approach, in fact they didn't use SP's at all.<br></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div></div><br> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><span class="gmail-m_4993947824586612435m_7759201291858962337gmail-"><br>> 3) Changes in the SQL code done in patches may be more complicated to track <br></span></div><br>Most of SQL code changes involve changes in a DAO too, so this shouldn't be an issue.<br></div></blockquote></span><div><div>It is !!! , changes in DAO are easy to track since it is a Java code , you are suggesting to write the SQL itself </div></div></div></div></div></blockquote><div><br></div></span><div>If you need to update a Java code together with SQL, then how much difference is in seeing the change in a single file or in two separate ones?<br>Frankly speaking I do not get what do you mean by "track". If you're about following what a DAO method does, then my approach simplifies the job.<br></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br>> 4) SQL Injection <div><br></div><div>Again, how other projects resolve the security problem? Internet is full of articles/blogs of why stored procedures should be avoided ("avoid <div></div>stored procedures" Google query returned ~6.6M results), so I guess there are some other approaches for the security issue if that exists.<span class="gmail-m_4993947824586612435m_7759201291858962337gmail-"><br></span></div></div></blockquote></span><div><br><div>"use stored procedures" Google query returned About 4,840,000 results </div><br></div></div></div></div></blockquote></span><div>How many of those add "do not" to the search term? Anyhow avoid gives more results...<br></div></div></div></div></blockquote><div><br>Again, I'm not a DB security expert, but IIUC using prepared statements with parameters (what my patches do) doesn't allow a SQL injection. On the other side the existing SearchDao.getAllWithQuery implementors are exposed to SQL injection attempts. Am I right?<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><span class="gmail-m_4993947824586612435m_7759201291858962337gmail-"><br>> 5) I think that SP performs better than SQL inside Java <br><br></span></div><div>Do we have a measurement of how much better? <br></div><div>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.<br></div></div></blockquote></span><div><br><div>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 ? <br></div><div>If people are not doing the right thing , we have gerrit for that, we can comment , nack , whatever to make the code good <br></div></div></div></div></div></blockquote><div><br></div></span><div>In a perfect word you're right, proper reviews should've stop that. But in the hard reality of oVirt code base it doesn't happen somehow... My proposal bases on the current situation of oVirt code that was created by using all Gerrit features.<br></div><div><div class="gmail-h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div></div><br> </div><div><div class="gmail-m_4993947824586612435h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br> </div></div><div class="gmail-m_4993947824586612435m_7759201291858962337gmail-HOEnZb"><div class="gmail-m_4993947824586612435m_7759201291858962337gmail-h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 27, 2017 at 11:09 AM, Eli Mesika <span dir="ltr"><<a href="mailto:emesika@redhat.com" target="_blank">emesika@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-size:large"><br></div><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Mar 27, 2017 at 1:56 AM, Moti Asayag <span dir="ltr"><<a href="mailto:masayag@redhat.com" target="_blank">masayag@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Sun, Mar 26, 2017 at 5:12 PM, Yevgeny Zaspitsky <span dir="ltr"><<a href="mailto:yzaspits@redhat.com" target="_blank">yzaspits@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div><div>Hi All,<br><br></div>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.<br>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. <br><br>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:<br><ul><li>no looking for a stored procedure name that is buried in the DAO class hierarchy<br></li><li>no looking for the SP definition</li></ul></div></div></div></div></blockquote><div><br></div></span><div>There are additional pros and cons for the suggestion to consider:<br><br></div><div>Pros:<br></div><div><ol><li>No need to run engine-setup after changing DB related code (in case of SQL inside Java). <br></li></ol></div><div>Cons:<br></div><ol><li>DAO files might become very long.</li><li>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:</li><li>An inflation of business entities to represent partial populated business entity or inflation of mappers inflation (this will be required for SP as well).</li><li>SQL code inside of Java: <br></li><ol><li>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).</li><li>No syntax highlighting when performing code review. i.e. I don't think reviewing a patch such as <a href="https://gerrit.ovirt.org/#/c/66729/10/packaging/dbscripts/network_sp.sql" target="_blank">https://gerrit.ovirt.org/#/c/6<wbr>6729/10/packaging/dbscripts/ne<wbr>twork_sp.sql</a> would be more clear inside a java file.</li><li>The user permissions management is implemented on DB level. That means that SQL will be more complex (more concatenated java lines). </li></ol><li>Stored procedure are cached by project's code. See SimpleJdbcCallsHandler.getCall<wbr>(), 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).</li><li>With the NamedParametersJdbcTemplate there is no use of the DbEngineDialect. What's the impact of it ? </li></ol><div>So besides 5 and 6, the rest is a matter of style. I'd like to hear more opinions from other members.<br></div></div></div></div></blockquote></span><div><br><div>I agree with all you wrote Moti<br></div><div>I don't think that looking in SQL in Java code is clear than looking in a SP code <br></div><div>Additionally <br></div><div>1) You will pass more data on the wire instead of calling a SP with parameters <br></div><div>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)<br></div><div>3) Changes in the SQL code done in patches may be more complicated to track <br></div><div>4) SQL Injection <br></div><div>5) I think that SP performs better than SQL inside Java <br></div><div><br></div><div>I see no real reason to replace the SPs with SQL code , SP is just a container for SQL code <br></div><br> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Regards,<br></div><div>Moti<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><div><div><div><div>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).<br>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.<br>IMHO the proposed approach would simplify adding new specific queries and by that would prevent a decent part of performance issues in the future.<br><br></div><div>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.<br></div><br>[1] - <a href="https://gerrit.ovirt.org/#/c/74456" target="_blank">https://gerrit.ovirt.org/#/c/7<wbr>4456</a><br>[2] - <a href="https://gerrit.ovirt.org/#/c/74458" target="_blank">https://gerrit.ovirt.org/#/c/7<wbr>4458</a><br><br></div>Regards,<br></div>Yevgeny<br></div>
<br></span><span>______________________________<wbr>_________________<br>
Devel mailing list<br>
<a href="mailto:Devel@ovirt.org" target="_blank">Devel@ovirt.org</a><br>
<a href="http://lists.ovirt.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.ovirt.org/mailman<wbr>/listinfo/devel</a><br></span></blockquote></div><span class="gmail-m_4993947824586612435m_7759201291858962337gmail-m_6843956363689495666m_4651098172073966859HOEnZb"><font color="#888888"><br></font></span></div><span class="gmail-m_4993947824586612435m_7759201291858962337gmail-m_6843956363689495666m_4651098172073966859HOEnZb"><font color="#888888"><div class="gmail_extra"><br clear="all"><br>-- <br><div class="gmail-m_4993947824586612435m_7759201291858962337gmail-m_6843956363689495666m_4651098172073966859m_4110374462987635130gmail-m_1038891330517396707gmail_signature"><div dir="ltr"><div>Regards,<br></div>Moti<br></div></div>
</div></font></span></div>
<br>______________________________<wbr>_________________<br>
Devel mailing list<br>
<a href="mailto:Devel@ovirt.org" target="_blank">Devel@ovirt.org</a><br>
<a href="http://lists.ovirt.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.ovirt.org/mailman<wbr>/listinfo/devel</a><br></blockquote></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>