Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

could Visitor pattern be usefull for configuration in Cf? #1601

Closed
rogierc opened this issue Apr 10, 2021 · 13 comments
Closed

could Visitor pattern be usefull for configuration in Cf? #1601

rogierc opened this issue Apr 10, 2021 · 13 comments

Comments

@rogierc
Copy link
Contributor

rogierc commented Apr 10, 2021

The Mule Coap connector has a set of configuration classes to define the CoAP servers, clients and CoAP endpoints to use in the Mule application. These classes organise all configuration-parameters in a logical structure to give the application developer a good user-experience.(Mule's development environment enables configuration in it's GUI using java-reflection). The Visitor design pattern is used to separate the configuration classes, that are structured for ease of use, from the proces of building Cf entities such as different types of endpoints and connectors.

Maybe this approach is usefull for Californium also. To explore this idea, I've put the configuration classes in a Cf-fork and removed Mule particulars. See: config package

The package contains an usage-example also. It shows how different visitors can be used to construct CoAP servers with different endpoint types. The visitors sub-package contains also the NetworkConfigGetVisitor and NetworkConfigSetVisitor that convert the new configuration-classes to or from the good old NetworkConfig.

The solution probably isn't complete but maybe enough to assess feasability of the idea.

@boaks
Copy link
Contributor

boaks commented Apr 12, 2021

My idea about a redesign of the configuration was to move common parts to element-connector and to extend that by the specific components (e.g. DTLS or TCP). Potentially also "network-interface" specific.

I'm not sure, which advantage the visitor pattern brings. Though some upstream projects will depend on that, I would try to get feedback from them as well.

@sbernard31 (leshan)
@sophokles73 (hono)
@kaikreuzer (openhab)

WDYT? Short feedback will be welcome.

@rogierc
Copy link
Contributor Author

rogierc commented Apr 12, 2021

The main advantage of the Visitor pattern is that the structure of the configuration itself is decoupled from components that use it internally. Different components can have their own view on (a part of) the configuration and don't have to 'know' its structure. This 'knowledge' is delegated to the visitor which extracts the needed configuration items. The structure of the configuration can be focused on ease of use (from application developer perspective), without imposing this structure on the internal components of Californium / Scandium. Different visitors can be defined for different purposes.

Because of the way Mule SDK works, I had to define configuration classes containing Mule annotations. These classes are scanned by Mule using reflection and are not allowed to depend on Californium classes. So, decoupling using the Vistor pattern was needed there. Could very well be that it isn't worth the effort in other usecases...

@sophokles73
Copy link
Contributor

Different components can have their own view on (a part of) the configuration and don't have to 'know' its structure. This 'knowledge' is delegated to the visitor which extracts the needed configuration items.

So, in any case, someone needs to know the structure (and semantics) of the configuration in the end. I do not really see the value of introducing the visitor pattern here as a general means of processing the configuration properties. It is still possible to decouple Cf's NetworkConfig class from (application specific) views on it by letting your visitor process the whole NetworkConfig class and create your application specific view from it (as @rogierc already seems to have done for Mule). I do not think that we need it internally to create Cf components as the configuration properties do not tend to change that often, or do they @boaks?

@boaks
Copy link
Contributor

boaks commented Jun 7, 2021

The NetworkConfig doesn't change too frequently.

The main pain on my side was, that this is located in the core, what makes it hard to use it for connectors, e.g. to configure the DTLS retransmission timeout. Therefore my idea is to push it into the element-connector and extend it by modules (e.g. dtls, tcp, core).

@rogierc
Copy link
Contributor Author

rogierc commented Jun 13, 2021

Would that decouple NetworkConfig from the classes being configured. Will users wil get specific classes/interfaces/builders to configure different components like endpoints and connectors?

One problem now is - I think - that NetworkConfig contains configuration for all these different components. That does not help the user with what configuration items need to be set for his use case. (the endpint/connector in use). Also items are not typesafe in NetworkConfig. The user has to check the component that get configured to know which configuration items are needed and what their type is.

The goal of the visitor pattern is not only to improve maintainability, but also being able to give the user a convenient and helpfull information structure of configuration items that is not burdened with 'technicalities' of how configuration is processed (like: it needs to be stored in a file and therefore everything is a String). Sure, this goal could be achieved using the visitor pattern, and also with other means. The main goal is IMHO to improve the configuration experience for the user.

@boaks
Copy link
Contributor

boaks commented Jun 15, 2021

Would that decouple NetworkConfig from the classes being configured. Will users wil get specific classes/interfaces/builders to configure different components like endpoints and connectors?

At least, I don't plan that. I hardly see a benefit in that.

One problem now is - I think - that NetworkConfig contains configuration for all these different components.

FMPOV, the main pain is, that NetworkConfig is in the core module. It has no means to be extended for different modules, nor can connector modules access it directly, because the design makes the core depending on the connectors, and not visa-verse. Moving a reduced NetworkConfig to the element-connector and provide a extension API would solve that pain. My current idea is still a general configuration, but the contained values are depend on the used and using module.

Also items are not typesafe in NetworkConfig. The user has to check the component that get configured to know which configuration items are needed and what their type is.

For me it seems to be somehow unclear, how users use that configuration. Editing the properties file? Prepare/adjust the config in code? My impression is, that it is much harder to know, what the "property does" (and knowing that, makes the type in many cases obvious), than to find out, which type is used. The purpose is too often bound to specification details and not that easy to understand. The only good thing is, that in many cases the defaults will do it and there is no general need to adjust them.

In my sum up:

  • there are question about that configuration, but not that much
  • using one "design-pattern" helps for sure to consume the API in that case, but not for other cases, where other "design-patterns" are in use. Seems to be very similar to topics like "logging interface".

@sbernard31
Copy link
Contributor

For me it seems to be somehow unclear, how users use that configuration. Editing the properties file? Prepare/adjust the config in code?

In Leshan library we prepare/adjust config in code.
In Leshan demo we use the propertie file.

My feedback about the design :
d1) NetworkConfig is about everything ? udp, dtls, coap, tls, http, tcp but in a same time there is also a DtlsConnectorConfig.Builder or CoapEndpoint.Builder. The responsibility of those classes sounds not so clear. Why some setting are in some builder and other in NetworkConfig? answering to those responsibility/scope questions could maybe help to define the new design.
d2) This class is in californium.core and handle concept which is not really "known" by this module. It sounds this introduce some unwanted kind of dependencies.

My feedback as a user about current NetworkConfig :
u1) It's not as easy to use as a classic builder because less easy to know all possible setting/type/default value. (with a builder and any IDE, autocomplete just help you to know that)
u2) You can only pass primitive type which could be limiting comparing to a classic builder (like DtlsConnectorConfig.Builder)
u3) The main advantage of the the current config is the possibility to serialize it in a properties files. (but could be done with another design for a subset of setting)

As a user It would be OK to me if I have several files(builder and properties) for configuration (like one for CoAP, one by connector, etc ...), but maybe this is possible to do have something clear with a unified config too. (harder to me to see how it could look like 🤔 )

HTH (a little bit 😬)

@sbernard31
Copy link
Contributor

(I'm a bit out of topic as my comment is not really related to the visitor pattern but this is more generics remarks about config redesign)

@rogierc
Copy link
Contributor Author

rogierc commented Jun 16, 2021

(I'm a bit out of topic as my comment is not really related to the visitor pattern but this is more generics remarks about config redesign)

The Visitor question is about which way to go forward with config redesign. So I don't think it's of topic. ;-)

@rogierc
Copy link
Contributor Author

rogierc commented Jun 23, 2021

d1) NetworkConfig is about everything ? udp, dtls, coap, tls, http, tcp but in a same time there is also a DtlsConnectorConfig.Builder or CoapEndpoint.Builder.

This class, that deals with 'everything' on one side and the different configuration items, builders and components that get configured on the other side, need to be loosely coupled. For maintainability reasons, but also because of different responsibilities. When not loosely coupled the designs need to account for responsibilities of other classes, need to be a compromise and will propably be less comfortable for the user.

The Visitor pattern is one way to realise loosely-coupled-ness. When not using this pattern which other design could be considered? (Haven't looked into #1648 yet.). Or is loosely-coupled-ness not considered needed here at all?

IMHO, the observations of @sbernard31 are symptoms of too much compromise in the design:

u1) It's not as easy to use as a classic builder because less easy to know all possible setting/type/default value. (with a builder and any IDE, autocomplete just help you to know that)
u2) You can only pass primitive type which could be limiting comparing to a classic builder (like DtlsConnectorConfig.Builder)
u3) The main advantage of the the current config is the possibility to serialize it in a properties files. (but could be done with another design for a subset of setting)

@boaks
Copy link
Contributor

boaks commented Jun 24, 2021

Or is loosely-coupled-ness not considered needed here at all?

My feeling is, a central point as AbstractConfigVisitor would require to be adapted for too many cases.

My current idea is still very close to NetworkConfig.
The builders are sometimes the result of the "layers" (e.g. scandium can not depend on core, and so NetworkConfig was no choice.), sometimes they offer the possibility of custom extension (e.g. the EndpointContextMatching or the AdvancedPskStore. In some places factories may have been used, but at least, if the main usage is a own custom implementation, a factory doesn't really pay-off for me.

I still think, that the view on the "right way" depends much on the way using it. Many people are just changing the value in Californium.properties, they don't adjust the values in their code. And those, who adjust the values in their code, would all love to have it done in their way. Unfortunately, only "one way" could be implemented, all others have to implement and maintain their glue- and converter-code.

@boaks
Copy link
Contributor

boaks commented Jul 30, 2021

I don't plan to adapt the new configuration to the "visitor pattern".
And my feeling is, no one else will do it either.
So, can we close this issue?

@boaks
Copy link
Contributor

boaks commented Aug 11, 2021

Feel free to comment, if this issue should be opened again.

@boaks boaks closed this as completed Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants