SearchService.generateConstraintParameter() is buggy on >1 Constraints

[zipwiz]

Look at SearchService.generateConstraintParameter(), line 464/466:

When building a parameter String, the } is only added finally, but not for the entries.

This leads to things like:

{"type":"category","values":["val1"],"exactMatch":"false"&constraint={"type":"category","values":["val1"],"exactMatch":"false"}

instead of

{"type":"category","values":["val1"],"exactMatch":"false"}&constraint={"type":"category","values":["val1"],"exactMatch":"false"}

Just fix near the very end of the method:

• Add adding } after the exactMatch part
• Remove the final adding of }

[zipwiz]

Addition:

I tried to work around the problem, but the resulting search URI with 2 constraint entries returns 0 entries, although an equivalent query directly fired via REST did what it should.

What may be the cause is the encoding of the parameters:

Because for URI parameter preparation a Map is used, it is not possible to have multiple "constraint" entries in the parameter map. To overcome this, multiple constraints are built as a String "{...}&constraint={...}" etc., which is put in the parameter map at key "constraint". When the URI is built, the & in this String is converted to % 2 2 instead of & amp ; . Maybe this causes the & before the 2nd constraint to be not recognized as parameter separator by the server?

I think the map as parameter container should not be used here and be replaced by something else which is able to carry multiple keyed values having the same key. This may of course have hard consequences for the whole implementation. An option could be a map containing collections as values optionally. This would require the map processor to accept String and Collection as well, which is difficult to model type-safe using generics (if not impossible).

[prb112]

We will look at the issue, and revert @projsaha @ManishKataria

[zipwiz]

Additional thoughts:

The usual way of preparing URI parameters from the parameter Map<String, String> is to generate something like "key=urlencode(value)" for every map entry and to add the first parameter using ? and all others using & . Path parameters are processed using XPath replacements.

In this case this leads to the second, third... "constraint" entry to be completely urlencoded, including the & URL parameter separator. Thus the & is not recognized as parameter separator any more, but as part of the data part of the first "constraint" URL parameter. Hence it is currently not possible to use >1 "constraint" entries.

For a useful API usage this is essential because the only way to decrease the result set cardinality on a big system is to use >1 "constraint" parameters. Only this way it is possible to fetch "communities with tag X"; if you search just for tag X with Scope "communities", then you receive all communities AND entities from inside communities, like community blogs, community forums, community forum topics etc.

[VineetKanwal1]

After analyzing the code, I think the problem is in constructing the parameters in SearchService.getResultsWithConstraint(), all the contraints are getting added with single key: public EntityList getResultsWithConstraint(String query, List constraints, Map<String, String> parameters) throws ClientServicesException{ // We can not use a map of constraints, since there could be multiple constraints but map can have only one key named constraint String formattedConstraints = generateConstraintParameter(constraints); if(parameters == null){ parameters = new HashMap<String, String>(); } parameters.put("constraint", formattedConstraints.toString()); return getResults(query, parameters); } In the above "formattedContraints may contain multiple constraints but since they are added with single key, they are treated as single value with "&constraint" being treated as part of the value.

[zipwiz]

+1. Next step?

[VineetKanwal1]

I am trying to test a solution. Will update soon.

[VineetKanwal1]

I am facing issues to test my fix because of some env issues. Can you please try it at your end. There are changes in 2 classes:

1. com.ibm.sbt.services.client.connections.search.SearchService.generateConstraintParameter(List)

   private String generateConstraintParameter(List constraints){

   StringBuilder formattedConstraints = new StringBuilder();
   if(constraints != null){
      for (int constraintsCtr = 0; constraintsCtr < constraints.size(); constraintsCtr++) {
          Constraint constraint = (Constraint) constraints.get(constraintsCtr);
          StringBuilder constraintParameter = new StringBuilder("");
          constraintParameter.append("{\"type\":\"").append(constraint.getType()).append("\"");
   
          if(StringUtil.isNotEmpty(constraint.getId())){
              constraintParameter.append(",").append("\"id\":\"").append(constraint.getId()).append("\"");
          }
   
          // Extract all values
          List<String> allValues = constraint.getValues();
          StringBuilder values = new StringBuilder();
   
          for (int valueCtr = 0; valueCtr< allValues.size();valueCtr++) {
              String value = (String) allValues.get(valueCtr);
              if(valueCtr == 0){
                  values.append("\"").append(value).append("\"");
              }else{
                  values.append(COMMA).append("\"").append(value).append("\"");
              }
   
          }
          constraintParameter.append(",").append("\"values\":[").append(values.toString()).append("]");
   
          constraintParameter.append(",").append("\"exactMatch\":\"").append(constraint.isExactMatch()).append("\"");
          formattedConstraints.append("}");
          if(constraintsCtr > 0){
              formattedConstraints.append("&");
          }
          formattedConstraints.append(constraintParameter.toString());                
      }           
   
   }
   return formattedConstraints.toString();

   }

2. com.ibm.sbt.services.client.ClientService.addUrlParameters(StringBuilder, Args)

   protected void addUrlParameters(StringBuilder b, Args args) throws ClientServicesException {

   Map<String, String> parameters = args.getParameters();
   if (parameters != null) {
      boolean first = !b.toString().contains("?");
      for (Map.Entry<String, String> e : parameters.entrySet()) {
          String name = e.getKey();
          if (StringUtil.isNotEmpty(name) && isValidUrlParameter(args, name)) {
              String value = e.getValue();
              if(value!= null && value.contains("&")){
                  String[] values = value.split("&");
                  for(String val : values){
                      first = addParameter(b, first, name, val);
                  }
              }
              else {
                  first = addParameter(b, first, name, value);
              }
          }
      }
   }

   }

As you can see from above, I am proposing to add provision to add multiple URL parametes with same name but different values by separating them with "&" delimiter. Please check at your end and see if it works. Thanks!

[zipwiz]

If it is coded like this, it will be impossible forever to send an ampersand character as part of any URL parameter value: All ampersand characters being part of such value will be treated as "mult-value delimiter". This would not be useful.

The problem indeed is the Map structure for the parameter set which does not allow for multiple URL parameters having the same name. This is a major design flaw at a very low level as it seems: Eventually it is required to work around the "default" URL parameter construction code and supply specific code instead. ("BYOU" = build your own URL ;-)

[VineetKanwal1]

I agree, it is a design issue. the parameter Map should be changed from String,String to String,List to allow multiple values for same name of parameter. Can you please first try this fix to check if it works?

[zipwiz]

I am currently "offline" with respect to the concerned project and will be able to check not before next week. The same holds for any work I can commit to the SBT code base.

Proposal:

(1) Do NOT change ClientService.addUrlParameters() now since this would impact the complete API in a very dangerous way.

(2) As a first step, fix the } issue like you already did in generateConstraintParameter(), that is: Ensure that every constraint parameter is terminated by }. Multiple constraint parameters will not work however, but at least this part of their construction will be fixed.

Finally: You may consider reworking the for(){} loops to follow this style:

    Collection<MyType> tcol = buildMyCollection();
    for (MyType t : tcol) {
        // do something with t
    }

But this is of course a matter of taste.

[zipwiz]

The other side:

Changing the handling of URL parameters in the API will impact the whole API very seriously. This would require a major refactoring effort. Thus it is essential to find out if there is any way to extend the behaviour without breaking the interface on this topic.

Basically your approach is just that, but this is not a suitable solution because it will damage important functionality. Just imagine what you would do if you want to send a parameterized URL as a parameter somewhere in the API: You would be required to encode the & by yourself, and even worse, it would not be sufficient to use "&" you would be required to use %22 (or what the actual code is). Currently the API is designed to automatically take care of such hassles, and I expect a great deal of code to rely on this behaviour.

Hence something else is needed here. I will also try to think about this problems, but its not simple.

[VineetKanwal1]

I am thinking if there is any delimiter which we can be sure will never be part of the value, we can use that instead of "&".

[zipwiz]

Which one should that be? Values are "unencoded URL parameter values", and they should be able to transport ANY character by design -- any invalid character will be encoded automatically as required..

[VineetKanwal1]

Ok. Yes, I agree. We need to think something different here but its not simple as it will involve design changes.

[zipwiz]

I imagine something else: The default way of building the encoded URL parameter string is just processing the map as it currently happens. If there is any way to bypass this but send some "pre-processed" lengthy URL to the ClientService methods together with an EMPTY parameter map, then the pre-processed URL should be used "as is" -- but in this case the whole URL preprocessing must happen, just with a special handling of multiple constraint parameters. This would be a "localized" solution which only impacts the Search API implementation, ideally only the constraint processing.

[zipwiz]

For this to achieve it would be necessary to extract the complete URL parameter processing logic step by step from the actual implementation, copy it into the SearchService context, than modify it to correctly handle the "constraints problem" and then send this URL to the ClientService WITHOUT the classic parameter Map -- because all parameters have already been incorporated into the URL by SearchService.

[zipwiz]

Not very nice, but prevents impact on the remaining API.

[VineetKanwal1]

yes, this will be safest in this situation.

[VineetKanwal1]

parameter map will be empty with all parameters including query and constraints already encoded and added to the search URL.

[zipwiz]

That looks promising. :-)

[VineetKanwal1]

Solution discussed above checked in to fix this issue.

[zipwiz]

I will check the result on April 24, 2015, and will give feedback. Implementation looks good.

[zipwiz]

Actually I would need something like "getMyResultsList" using SearchUrls.MYSEARCH instead of SearchUrls.SEARCH. This however is an enhancement discussed in #1675 . Because it is impossible to reach the required internal logic from outside I must add such a method to my local code base and test it using this extended implementation. The core logic however is unchanged, and thus the results will be valid. I will report the results soon.

[zipwiz]

Works. See #1677 and #1675 however:

• For #1675 I have provided #1678 .
• For #1677 I have provided #1678 also.

[zipwiz]

Is this merged into master by https://github.com/OpenNTF/SocialSDK/commit/da01a8d98b4990e4d1e32ec7b23d9c031b2d6b53 ? Then it could be closed.

[prb112]

yes