Initial project files

Change-Id: I403713686bb7a5842feb211241de5f2530e0a457 Signed-off-by: Martin Lowe martin.lowe@eclipse-foundation.org

Created by: mbarbero

I like functional for those simple stuff. What do you think about:

return docs.stream().map(mapper::toBean).collect(Collectors.toList());

Created by: mbarbero

Same:

List<SolrInputDocument> solrDocuments = beans.stream().map(mapper::toDocument).filter(Objects::nonNull).collect(Collectors.toList())

Created by: mbarbero

I don't think this class is required to be mutable. Once the fields are set in the constructor, I don't see the use case for modifying them. I'd suggest to remove all setters and and set all fields as final

Created by: mbarbero

while reading addParam and setParam, I've the feeling that they should be merged into somehting like

public void addParam(String key, String value) {
    if (StringUtils.isBlank(key)) {
         throw new IllegalArgumentException(EMPTY_KEY_MESSAGE);
     }
    if (value == null) {
        throw new NullPointerException("value must not be null");
    }
    map.computeIfAbsent(key, k -> new ArrayList<>()).add(value);
}

and add the unsetParam(String key) method as mentioned above.

Created by mbarbero

Review: Changes requested

Pleas have a look at the couple of comments I made. Thanks.

Created by: mbarbero

Never iterate of keys of map to do a get on each key. It's very ineffective.

Better iterate on the Map.Entry<K, V> return type of entries() (or even better, use forEach)

doc.getFieldValuesMap().forEach((key, value) -> result.setField(key, value));

The reason I do this is a limitation of SolrJ when handling a SolrDocument. The map returned by getFieldValuesMap() is an anonymous implementation of the Map interface with the following for entrySet():

      @Override
      public Set<java.util.Map.Entry<String, Collection<Object>>> entrySet() {throw new UnsupportedOperationException();}

While it does require the extra operations of retrieving the value for the key, it is the best available option.

Created by: mbarbero

To avoid this whole if cached, return; otherwise create, cache and return , I would define the caching service so that it leverages the Cache::get(K, Callable<? extends V>) method. See https://guava.dev/releases/snapshot/api/docs/com/google/common/cache/Cache.html#get-K-java.util.concurrent.Callable-

With the comment below about adding a generic type to the CachingService and moving getCacheKey to CachingService, the get method would get 3 parameters: String, QueryParams and Callable<? extends T>

Created by mbarbero

Review: Changes requested

Please see my comments.

I'm going to cut off new dev for this PR after this, and will create PRs for new features like oauth services, and creating more resources and the like

Created by: mbarbero

String::split is quite dangerous (e.g. what the result spliting of ",," or ", ,, "). For all split, I'd suggest you use https://github.com/google/guava/wiki/StringsExplained#splitter

It's not a blocker for this code, but keep that in mind in the future.

Created by: mbarbero

Never told you before, but very good reflex to return a copy ;)

Created by mbarbero

Review: Approved

Be careful with String.split, otherwise LGTM.

Thanks for all the refactoring. This is good work!

Will do!