Fix LDAPFilter::Match bug #228

ksubramz · 2017-08-17T14:49:23Z

Addresses #227

Fix the bug by taking into account the length of the keys. Fix exception documentation of LDAPFilter::Match function. Add tests to verify the fix. Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>

Add test to test exception for case variants of same key.

codecov-io · 2017-08-17T14:49:40Z

Codecov Report

Merging #228 into development will increase coverage by 0.76%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##           development     #228      +/-   ##
===============================================
+ Coverage        82.69%   83.46%   +0.76%     
===============================================
  Files              186      186              
  Lines            13138    13186      +48     
===============================================
+ Hits             10865    11006     +141     
+ Misses            2273     2180      -93

Impacted Files	Coverage Δ
framework/src/util/Properties.cpp	`89.55% <100%> (+4.7%)`	⬆️
framework/test/driver/LDAPQueryTest.cpp	`98.18% <100%> (+0.88%)`	⬆️
framework/src/util/SharedLibrary.cpp	`65.27% <0%> (-25.7%)`	⬇️
framework/include/cppmicroservices/SharedData.h	`78.68% <0%> (-16.4%)`	⬇️
framework/test/driver/ConcurrencyTest.cpp	`81.48% <0%> (-7.41%)`	⬇️
framework/test/bundles/libSL3/ActivatorSL3.cpp	`90% <0%> (-4.12%)`	⬇️
framework/test/bundles/libSL1/ActivatorSL1.cpp	`92.59% <0%> (-3.71%)`	⬇️
framework/src/service/ServiceReferenceBase.cpp	`74.82% <0%> (-2.05%)`	⬇️
framework/src/bundle/BundleRegistry.cpp	`85.45% <0%> (-0.61%)`	⬇️
framework/src/service/ServiceObjects.cpp	`67.36% <0%> (-0.34%)`	⬇️
... and 23 more

jeffdiclemente

The OSGi spec for public interface Filter (Section 10.1.16) only states that matchCase throws an exception. What's the reason for all the other match methods throwing?

ksubramz · 2017-08-17T17:36:13Z

Addressing Jeff's comment, the reason all the Match methods are throwing because they all construct temporary Properties objects. And both the exceptions may get thrown in the constructor of Properties.

jeffdiclemente · 2017-08-17T18:18:30Z

alright, closing the loop; I talked to Krish offline and we resolved my confusion on why some match methods throw and others don't. Its because a throw happens in the BundleContext::RegisterService method if case variants are found.

saschazelzer

Looks good except that minor doc issue.

saschazelzer · 2017-08-17T18:39:48Z

framework/include/cppmicroservices/LDAPFilter.h

@@ -130,6 +130,10 @@ class US_Framework_EXPORT LDAPFilter {
   *        in the match.
   * @return <code>true</code> if the bundle's properties match this
   *         <code>LDAPFilter</code> <code>false</code> otherwise.
+   * @throws std::runtime_error If the number of keys of the <code>bundle</code>
+   *         exceeds the value returned by std::numeric_limits<int>::max().
+   * @throws std::runtime_error If the <code>bundle</code> contains case variants


This can never happen, since the Match() function operates on the bundles headers, which is a case insensitive map already.

jeffdiclemente · 2017-08-17T18:33:24Z

framework/include/cppmicroservices/LDAPFilter.h

@@ -130,6 +130,10 @@ class US_Framework_EXPORT LDAPFilter {
   *        in the match.
   * @return <code>true</code> if the bundle's properties match this
   *         <code>LDAPFilter</code> <code>false</code> otherwise.
+   * @throws std::runtime_error If the number of keys of the <code>bundle</code>


the keys are part of the bundle's properties and not the bundle itself.

jeffdiclemente · 2017-08-17T18:33:28Z

framework/include/cppmicroservices/LDAPFilter.h

@@ -130,6 +130,10 @@ class US_Framework_EXPORT LDAPFilter {
   *        in the match.
   * @return <code>true</code> if the bundle's properties match this
   *         <code>LDAPFilter</code> <code>false</code> otherwise.
+   * @throws std::runtime_error If the number of keys of the <code>bundle</code>
+   *         exceeds the value returned by std::numeric_limits<int>::max().
+   * @throws std::runtime_error If the <code>bundle</code> contains case variants


the keys are part of the bundle's properties and not the bundle itself.

jeffdiclemente · 2017-08-17T18:35:34Z

framework/include/cppmicroservices/LDAPFilter.h

@@ -142,6 +146,10 @@ class US_Framework_EXPORT LDAPFilter {
   *        in the match.
   * @return <code>true</code> if the <code>AnyMap</code>'s values match this
   *         filter; <code>false</code> otherwise.
+   * @throws std::runtime_error If the number of keys of the <code>dictionary</code>


If the number of keys of the dictionary

sounds a little awkward. I'd suggest:

If the number of keys in the dictionary

jeffdiclemente · 2017-08-17T18:36:13Z

framework/include/cppmicroservices/LDAPFilter.h

@@ -154,6 +162,10 @@ class US_Framework_EXPORT LDAPFilter {
   *        in the match.
   * @return <code>true</code> if the <code>AnyMap</code>'s values match this
   *         filter; <code>false</code> otherwise.
+   * @throws std::runtime_error If the number of keys of the <code>dictionary</code>


I'd suggest:

If the number of keys in the dictionary

Amend API documentation

saschazelzer

Just one other minor thing.

saschazelzer · 2017-08-17T19:25:15Z

framework/include/cppmicroservices/LDAPFilter.h

@@ -130,10 +130,8 @@ class US_Framework_EXPORT LDAPFilter {
   *        in the match.
   * @return <code>true</code> if the bundle's properties match this
   *         <code>LDAPFilter</code> <code>false</code> otherwise.
-   * @throws std::runtime_error If the number of keys of the <code>bundle</code>
+   * @throws std::runtime_error If the number of keys of the bundle's properties


We need to be careful about the terminology here. It is the bundle's manifest headers that are checked and matched. The return doc above also users the wrong properties term.

Improve API doc based on feedback

saschazelzer · 2017-08-18T14:14:32Z

framework/include/cppmicroservices/LDAPFilter.h

-   * @throws std::runtime_error If the number of keys of the bundle's properties
-   *         exceeds the value returned by std::numeric_limits<int>::max().
+   * @throws std::runtime_error If the number of keys of the bundle's manifest
+   *         properties exceeds the value returned by std::numeric_limits<int>::max().


I am sorry, but elsewhere we really use the term headers (the Match() implementation really calls bundle.GetHeaders()) for the manifest data, not properties. properties are accessed via the bundle context and are framework properties. manifest properties now is even more confusing in my opinion.

No issues :). The reason I chose the wordings was because the short blurb of this API method says Filter using a bundle's manifest properties. I'll change as per your feedback.

Oh okay. Indeed, we used properties before the introduction of framework properties and the need to differentiate these. So the short blurb should be updated as well. Thanks!

Change API doc per feedback

ksubramz added 2 commits August 16, 2017 15:48

Fix LDAPFilter Match bug

e68aa18

Fix the bug by taking into account the length of the keys. Fix exception documentation of LDAPFilter::Match function. Add tests to verify the fix. Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>

Fix LDAPFilter Match bug

008e9ad

Add test to test exception for case variants of same key.

ksubramz requested review from saschazelzer, jeffdiclemente and karthikreddy09 August 17, 2017 14:49

jeffdiclemente reviewed Aug 17, 2017

View reviewed changes

saschazelzer requested changes Aug 17, 2017

View reviewed changes

jeffdiclemente requested changes Aug 17, 2017

View reviewed changes

Fix LDAPFilter Match bug

cdfc7d1

Amend API documentation

saschazelzer requested changes Aug 17, 2017

View reviewed changes

Fix LDAPFilter Match bug

4537d7c

Improve API doc based on feedback

saschazelzer requested changes Aug 18, 2017

View reviewed changes

Fix LDAPFilter Match bug

f6c049c

Change API doc per feedback

saschazelzer approved these changes Aug 18, 2017

View reviewed changes

jeffdiclemente approved these changes Aug 22, 2017

View reviewed changes

karthikreddy09 approved these changes Aug 23, 2017

View reviewed changes

ksubramz merged commit 90fa8ce into development Aug 24, 2017

saschazelzer deleted the 227-ldapfilter-match branch June 14, 2018 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LDAPFilter::Match bug #228

Fix LDAPFilter::Match bug #228

ksubramz commented Aug 17, 2017

codecov-io commented Aug 17, 2017 •

edited

Loading

jeffdiclemente left a comment

ksubramz commented Aug 17, 2017

jeffdiclemente commented Aug 17, 2017

saschazelzer left a comment

saschazelzer Aug 17, 2017

jeffdiclemente Aug 17, 2017

jeffdiclemente Aug 17, 2017

jeffdiclemente Aug 17, 2017

jeffdiclemente Aug 17, 2017

saschazelzer left a comment

saschazelzer Aug 17, 2017

saschazelzer Aug 18, 2017

ksubramz Aug 18, 2017

saschazelzer Aug 18, 2017

Fix LDAPFilter::Match bug #228

Fix LDAPFilter::Match bug #228

Conversation

ksubramz commented Aug 17, 2017

codecov-io commented Aug 17, 2017 • edited Loading

Codecov Report

jeffdiclemente left a comment

Choose a reason for hiding this comment

ksubramz commented Aug 17, 2017

jeffdiclemente commented Aug 17, 2017

saschazelzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschazelzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 17, 2017 •

edited

Loading