Usage of reflection in CriterionToPredicateConverter

[jimmarino]

@paullatzelsperger Why does this class need `field(String fieldName, Asset asset)1?

[paullatzelsperger]

the only alternative I can think of would be a switch cascade (or an if-else) to get the appropriate field explicitly. The current implementation is more resilient toward refactoring of the Asset class, and it can be easily enhanced to a CriterionConverter<Predicate<T>> which might come in handy in the future as our "query-dsl" matures.

[jimmarino]

@ndr-brt has a good point. Should there be any other fields on Asset and/or should they be picked up in this manner? I like pushing the extensibility data explicitly into the properties.

[paullatzelsperger]

I think the point it that, on the Asset class, besides id, name and version, every property will be inside the properties map, so maybe a switch between these 3 fields and the properties lookup as the default branch could fit better

To be precise there is also the contentType field. We need to be extra careful that, should we ever add another field to Asset, we must adapt that switch as well. That's a potential for nasty bugs.

Another option would be to actually have no fields on Asset class besides the properties, just named getters, for example for version`:

public String getVersion(){
    return properties.get(VERSION_PROP_KEY);
}

Analogously, the builder would simply set that property:

public Builder version(String version) {
      this.properties.put(VERSION_PROP_KEY, version);
      return this;
}

[ndr-brt]

Another option would be to actually have no fields on Asset class besides the properties, just named getters, for example for version`:

public String getVersion(){
    return properties.get(VERSION_PROP_KEY);
}

Analogously, the builder would simply set that property:

public Builder version(String version) {
      this.properties.put(VERSION_PROP_KEY, version);
      return this;
}

This could be the solution, I see no disadvantages

[jimmarino]

+1 for that

[paullatzelsperger]

See Issue #183 .