I don't think this is a good example to justify adding new spec API here. The filters here are hard-coded, well tested already and never change. The overhead of using a FilterBuilder in this case is like using a StringBuilder to build up a static final string vs just quoting the complete string in a single final constant that can get compiled into the class vs built up at runtime.

FilterBuilder is useful for cases that need to dynamically create filters based on some external input. I believe that usecase impacts a relatively small group of developers.

Then maybe org.eclipse.core.internal.runtime.Activator is a better one?
There the filter is created with the protocol on each call:
https://github.com/eclipse-equinox/equinox/blob/master/bundles/org.eclipse.equinox.common/src/org/eclipse/core/internal/runtime/Activator.java#L271-L279

But for me this is not mainly about performance but about improving the readability of the code and simplicity to construct new ones. Although you are right that one usually does not create new ones very often (at least I don't).

Perhaps, but if I was to rewrite that code I would opt to use a single ServiceTracker<URLConverter, ServiceReference<URLConverter>>
with a simple filter "(objectClass=" + URLConverter.class.getName() + ")" and that tracker maintains a Map<String, List<ServiceReference<URLConverter>> where the key is the protocol and the list is a service rank ordered list of converters for that protocol. Then when a specific protocol was requested it would call ctx.getService(ServiceReference) with the highest ranked reference for the protocol from the map.

That seems better to me than maintaining a map of trackers based on user input for protocols that may never exist.

I really doubt that performance is an issue here, but for sure if that's an impact users can still use "faster" Strings.

Also of course one can always argue that it is constant in some way and testes, but the same way one can argue against static final constants as they are constant why extract them and thing about a good place/name and so on if they are constant and well testet?

It is also about readability and maintainability and generally I'm fine spending a few microsecconds for this. A good example is this one:

https://github.com/eclipse-equinox/p2/blob/55bc99b583490aec6a703ed34baf39d489f84e96/bundles/org.eclipse.equinox.p2.touchpoint.eclipse/src/org/eclipse/equinox/internal/p2/touchpoint/eclipse/LazyManipulator.java#L28-L33

this is really hard to read, hard to understand and horrible to maintain and shows the usual anti-pattern of catch/ignore exception because we know it could NEVER happen

} catch (InvalidSyntaxException e) {
// Can't happen we are writing the filter ourselves
return null;
}

the bad thing is just if someone accidentally mess up the filter a NPE is thrown somewhere (or maybe nothing happens at all if code thinks null mean missing service) ...

the bad thing is just if someone accidentally mess up the filter a NPE is thrown somewhere (or maybe nothing happens at all if code thinks null mean missing service) ...

To solve that I suggest we consider changing InvalidSytaxException to extend RuntimeException.

What is a "semantically" invalid filter? A Filter either match or do not match and I'm not aware of any assertion regarding the "content" of the filter.

Strings are immutable, builders are not.

While the Builder is mutable, it is expected that a user would need to call e.g.

Filter myFilter = createFilter().eq(Constants.OBJECT_CLASS, "test").build();

and then use the Filter object (that is immutable) to pass it along.

We did not go into details on overloading core APIs to take a FilterBuilder where Strings are used today (e.g. BundleContext.addServiceListener(ServiceListener, String)).

I always wondered why there is not a method BundleContext.addServiceListener(ServiceListener, Filter)) as it seems obvious like with ServiceTracker...

I always wondered why there is not a method BundleContext.addServiceListener(ServiceListener, Filter)) as it seems obvious like with ServiceTracker...

There are two reasons that we discussed in the spec call relating to accepting foreign implementations of the Filter implementation :

1. Framework can depend on always using its own Filter implementation and make optimizations for service event delivery and lookup
2. Framework can trust its own implementation to do the right thing

FrameworkUtil.createFilter was a mistake in my opinion. The only core API where a Filter is passed is the constructor of ServiceTracker. But the actual implementation of the Filter is not used for any match at all. Instead it calls toString and then that is parsed again by the framework to ensure it has its own impl of Filter. For historical reference, FrameworkUtil.createFilter was added so the implementation of the various framework permissions could create Filter objects in their implementations. But we could have instead made that Filter impl used for permission implementations to be a package private class to parse the filter string input for the permission implementation. Then when we finally gut all permissions because no supported Java will have any ability to have a security manager then we could simply remove this non-API Filter impl from the spec classes. But instead we are left with a public facing createFilter util method for all time. I am not convinced adding yet another way to create a Filter object is a good idea.

Even though its seldom, when needed Filters are really usefull, and it seems odd especially for ServiceTracker and alike not to simply call "match" on the provided filters...

IIRC, ServiceTracker with a Filter used to call match on the provided filter and then register a listener with no filter with the framework. This side-stepped any optimizations the framework could do to avoid full filter evaluation against every service registered with the framework during tracker open or during any service event. It also had issues because service listener hooks wanted more information on what services a bundle requires by looking at https://docs.osgi.org/javadoc/osgi.core/8.0.0/org/osgi/framework/hooks/service/ListenerHook.ListenerInfo.html#getFilter--

Before the ServiceTracker impl used no filter registered with the framework, resulting in ListenerHook.getFilter returning null. Some scenarios needed to have more information on what service properties a bundle required. @bosschaert I think that was important for stuff like remote services, correct?

Correct, remote service want to know what services are being looked for so that they can be discovered and registered on-demand.

The main problem with such "small helpers" is that it tends to be not used because it is "non-standard" and people have to fiddle with licensing deployment and release issues, while something provided by default is likely to be used more.

Beside that there are the following issues:

1. Either we must fall back to strings again that are then pared again to a filter or the framework must be happy to accept any Filter implementations an not only framework provided ones
2. Actually efforts are duplicated, this library must implement the filter syntax, and the framework must implement the syntax e.g look at Equinox FilterImpl it actually has object representations of an/or/eq ... so this could be reused for a Builder, actually the Parser is already a builder that works an a stream of characters as input instead of method calls.

What I think could work would be that there is one FilterFactory maintained and provided by the osgi-specification itself and equinox/felix/... could simply use this, actually all Filter handling is not OSGi Framework specific and not bound to the framework istself ... Equinox effectivly always delegates to FilterImpl.<somestaticmethod> anyways...

It is just an idea. That things are not used because they are "non-standard" is just a hypothesis.
People also tend to use tons of jars from Maven Central and very often don't care of they just use one class of a JAR.
Then I follow BJ's suggestion to provide a design document.

It is just an idea. That things are not used because they are "non-standard" is just a hypothesis. People also tend to use tons of jars from Maven Central and very often don't care of they just use one class of a JAR.

From my limited experience it depends and the behavior is not consistent. Some do really care about 'official' dependencies and some don't.

I think the concern is foreign Filter impls which cannot be trusted and are not optimized for the framework impl. The string format of a filter is the serialized form of a Filter and is the best and most basic exchange format. The receiver of the string filter is then free to deserialize it as they wish.

That's right. But in that case wouldn't it be possible, with the approach suggested already that the Framework creates the Filter-Builder/Factory, that receivers of such strings check the actual class of the filter object and throw a IllegalArgumentException (or alike) if it is not the Frameworks implementation? There could be a Framework internal method like requireEquinoxFilter() that does that, just like there is for example Objects.requireNonNull().

But why this unnecessary complication? The whole idea of a builder for filters is a library issue not a specification issue. I don't see this as a productive use of the project's time. The only useful thing from this discussion would be to rebase InvalidSyntaxException on RuntimeException to simplify code authoring around calling methods which are documented to throw InvalidSyntaxException. Such a change is simple, backwards-compatible, and doesn't bloat the API with overloads and a builder type. It is easy to explain in the specification (it basically requires zero changes to the specification text).

I am a strong -1 on adding a builder type and overloading methods to take Filter objects.

Instead of specifying it in the OSGi spec we could also incubate that for example in Equinox (and/or others) and see if it works well and if it is used. In Eclipse-Platform we have a three years deprecation/removal policy (and if that applies to Equinox as well), we could remove it again if it goes really bad or if it goes very well, it can evolve to be part of the OSGi spec.
This would probably also be less work in the first step because we can start with the technical part first and have some experience when it comes to specifying it.

This will always return a "good" framework managed filter but prevents parsing the filter again if it is already of the desired type.

Why allow foreign filters to be passed in if they are not going to be called? I realize we are in that situation with ServiceTracker, but I would not think we should repeat that mistake.

I think it must not be allowed, but if one absolutely want to be "safe" it would be an option, but IllegalArgumentException would be fair enough as well.

Passing strings with semantics is really bad as there is only runtime-checking and no compile-time (or even IDE-typing-time) checks so if a string is not a string (=some text without semantics that needs be parsed) I always would prefer an Object with an (interface) type.

If InvalidSyntaxException is changed to a RuntimeException and a builder is available (perhaps not from core) that only produces a syntactically correct filter String then it seems to solve the concern without overloading methods that take String filters to also take Filter objects.

In general I find this a good suggestion. It could be considered to name the method equal() instead of eq() and to make not() a static method so that it can be read more human readable ( not(equal(..))).
Also both would be possible. For example java.util.function.Predicate has both, a default method negate() and a static method not().

It could also be considered to have specialized equals for common cases like one for objectclass filter, that accepts a Class:
hasClass(Person.class) or isOfClass(Person.class). At the moment that is often programmed like:
"(objectclass=" + Persion.class.getSimpleName() + ")".
The method name should clarify that the filtered object has to be of that type and must not be of a sub-type (or is that possible as well? I have to admit I don't know that).

The problem with static methods is that they can't be overridden by the implementation by a more efficient ways, so the static ones should really only be an entry point to get an initial filter and then operate on the filter object itself.

That's right. But if there are both, then the static not(filter) could just call not/negate() on the passed filter object and return the result. This way the default not/negate() method can be overriden, but could still have the syntactic sugar.

Thanks, probably this can be contributed to OSGi then so have it in core?