> On June 14, 2016, 8:50 p.m., Hao Hao wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java, > > line 106 > > <https://reviews.apache.org/r/48576/diff/1/?file=1415444#file1415444line106> > > > > I feel it would be more clean to add the logic to use the default > > authority here instead of at line 78. We would want to ensure > > makeFullQualifiedURI will return a full qualified URI.
PathUtils has two public versions of impliesURI(): * impliesURI(URI,URI) - the one with my fix. It never calls makeFullQualifiedURI(), so if this method is called directly, fixing makeFullQualifiedURI() won't help. * impliesURI(String,String) does call makeFullQualifiedURI() and then delegates to impliesURI(URI,URI). If only this method is ever called, it is true that fixing makeFullQualifiedURI() is all we need. Since both flavors of impliesURI(,) are public, I put the fix into impliesURI(URI,URI) But I agree that makeFullQualifiedURI() should set all the missing URI elements to the default. Currently it does not, because it uses Path.isAbsoluteAndSchemeAuthorityNull() which returns true only if BOTH scheme and authority are null. I can fix this logic as well, but I still need to keep the implemented fix in impliesURI(URI,URI). Alternatively, the code can be refactored, so that both flavors of impliesURI() go through the common logic of ensuring fully qualified URI. Let me know which way you prefer. - Vadim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48576/#review137580 ----------------------------------------------------------- On June 11, 2016, 2:33 a.m., Vadim Spector wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48576/ > ----------------------------------------------------------- > > (Updated June 11, 2016, 2:33 a.m.) > > > Review request for sentry, Hao Hao and Sravya Tirukkovalur. > > > Bugs: SENTRY-1311 > https://issues.apache.org/jira/browse/SENTRY-1311 > > > Repository: sentry > > > Description > ------- > > SENTRY-1311: Improve usability of URI privileges by supporting mixed use of > URIs with and without scheme > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java > 3b9336c3e0d70b03dd6e2bf27919e2510031addd > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 1606b6d97c6696328906100eb10464fdc2fa19e8 > > Diff: https://reviews.apache.org/r/48576/diff/ > > > Testing > ------- > > > Thanks, > > Vadim Spector > >
