Transparency of Apps - Recent changes in Design put stability at risk

Rocket.Chat Apps
1

[tl:dr] There is a significant change in the apps-area which imho puts the success of the whole apps-thing at risk.

Trigger

As per a recent commit to the Apps-Framework, the architectural foundation of the apps was shaken.

Motivation - and why this is important

The principle of dependency inversion (aka “dependency injection”, “DI”) was a foundation when designing the interfaces of the apps: The framework shall be in control (e. g. of where an implementer of the framework can do what). Each method should get access only to features and functions which the framework intends.
This makes it possible for the framework to determine the app’s capabilities.

Defined app responsibilities

E. g. a rocket.chat-app implementing only IPreMessageSentExtend and IPreMessageSentPrevent has some defined capabilities: It may prevent that a message is sent and it may extend this message (e. g. add an attachment), but for sure it’s not going to change the body of a message.
In contrast, a bad words filter app implementing IPreMessageSentModify obviously has to do exactly this.
Why is this important? Think about a company installing an app from an app store: You want to be sure what the app can do, similarly to installing an app on mobile: “this app accesses your microphone” - or in your browser: “this extension is going to read all your traffic”.

By providing a global helper, this mechanism is bypassed. Unless you parse the code (and believe me, parsing is not enough) you will never know what an app might do.

From an enterprise-perspective, such an app-store is a no-go.

Development quality

Seen from a developer’s perspective, it’s about ease-of-implementation, static validation at build time and testing. We could argue about the first one (which I’ll do in the next paragraph), but static validation of the artifact consumed and mockability which heavily influences testability is undoubtly better with DI.

But there is simplicity first…

Currently, the apps’ design features a lot of interfaces with long names and long parameter lists. True. But is this really the bad part? Is it really more comfortable to have a constructor injected (“app global”) dependency?
In my opinion, the initial approach is way better. Ok, of course, I’m biased (contributed parts of it).
Thus, let me quickly explain why @bradley.hilton and I went for this approach back then

Signature driven

Often, a developer starting with an environment looks at the API first - and may look at the documentation later. A fundamental question when implementing something which runs in a bigger context is “where do I put my code”. For this, interfaces and their methods are not only a guideline, but since they are available at runtime, they are the truth.
The interfaces in the Apps are crafted so that each interface defines the timepoint while the method defines the permissions which are granted to the logic. This naturally splits logic. Do I really need to execute the potentially expensive logic? => put some validation code into the check{Timepoint} method. This does not only improve overall quality, it also allows the framework to optimize.

Comfort

But the signatures are long

Yes, they are - kind of. But therefore, you don’t even need to import a dependency - the dependency is injected, it’s just there, use it. How more comfortable could it be?

Verdict

Alright, I hope I presented the motivations properly and you now at least know what my opinion is. There may be other opinions as well - and many of them are surely equally valid.
My opinion is coming from a now 15 year long (you may call me “old” - yack!) background, long time working with business software and frameworks at an enterprise grade in various languages. It may not be the right one, but believe me it’s not a shot into the blue.

My verdict given that there are other arguments

Variant 1: Be consequent

By allowing each method to do whatever you want with the context they run in, you basically can get rid of the signatures. So if you go down this route, also kill the signatures (incompatible change) and in the same step also kill the separated check and execute methods. This will have the framework in control of when an app is executed, but will result in potentially more messy app implementations.

Variant 2: Provide a good documentation

If you got feedback about difficulties kickstarting an app, provide more

  • Boilderplate
  • Samples
  • Code-near documentation

Imho, the later one is more promising when it comes to maintenance - and when it comes to perceiving Rocket.Chat as a tool really made for enterprises.

2

@ojaegle I understand your points, but I have a different view, lets see if I can explain my self.

Defined app responsibilities

We are not changing the interfaces who define the responsibilities, but I don’t think we should prevent the developers to have access to the accessors or modifiers, I see the point where the Extend vs Modify will not make sense anymore since you can have access to the Modify inside an Extend method, but then I have a second point, for me, these 2 ways to edit a information makes sense when you are coding but would not be expected to be a permission, I can’t see we showing to the final users a list of permissions where an app can only extend a message, what that really means? The permission system may be read/edit/create, more than that may be too complex IMO.

The permissions should not be checked by interfaces implemented, I don’t think they would be enough, what I see is a list of permission strings you should add to your package.json file listing what capabilities your app will request, you can access any of that capabilities or even check if one of them was granted, but if you try to access some capability that wasn’t requested/granted you will receive an error or exception, that is what you need to do when developing for iOS.

Development quality

The change is not related we trying to make thinks simpler, but to empower the developer. I saw many times developers trying to do something that wasn’t possible because we didn’t pass some accessor in some method. If an slaschcommand have access to the persistence in a command executor why he can’t have access to that inside the command constructor. For me, the access to the API should be limited by the permission system and not by the thing I’m implementing.
Why when implementing a PostMessageSent I don’t have the Extend and Modify options? So there I can’t define a permission logic via interface usage.

I agree that check and execute are both things we could remove, I would not touch then right now because it’s not clear it would be good for everyone and it would create a break change. But it’s what I’m thinking for the future.

For sure we need to have a better documentation, samples and boilerplates, we are focusing on creating apps for Atlassian now, it’s our priority to attend the companies migrating from HipChat, but after that I want to put efforts on this side of the project.

I see the points you added here and your fears, I saw some developers didn’t understanding the complexity of our apps system as well and I received lots of questions why somethings can’t be done and I’m seeking for a better balance between quality and flexibility.

What do you think?

3

@rodrigo.nascimento thanks for the elaborate response, this type of discussion is exactly what I wanted to trigger!

Major subject to discussion was the approach to limit capabilities: “interface driven” (the signature defines the capabilities) vs. “manifest driven” (some other metadata defines what can be done). I agree with not all capabilities being enforceable using an interface.

Benefits of a manifest

Let’s make an example with HTTP-communication: For an enterprise, it’s important to know to which domains an app communicates via HTTP. This is important not only to set up the proxy properly, but also has implications on data security.
You could use a manifest to define the urls to which the app wants to talk to. The HTTP-object which you inject can then be configured accordingly, so that it raises a runtime exception once a different URL is addressed.
You can’t enforce this mechanism via the interface, since the HTTP-methods are structurally independent from the url which they target.
What I wanted to say with the above paragraph: agreed: For fine granular permissions, you have to utilize a manifest.
Remark: Whether you really want to have a separate file or annotations has been a looong subject to discussion throughout the developer ages, but this is only a matter of implementation - I consider both as manifests.

Benefits of interface driven capabilities

When implementing a framework (which utilizes DI to operate the components running in it), a crucial information for the framework runtime is idempotency: Which method of the component can the framework repeatedly call without the method modifying the state of the application?
This is important since state changes can not easily be parallelized, potential caches need to be updated, it’s an expensive operation.
Once you make a state modifying operation globally available, the framework loses this insight. Also, a manifest does not help here: A manifest defined in a separate location (e. g. a file) is independent from the context in which the capabilities is accessed.
Once a I...Modify is only available in certain methods, the framework can statically verify the capabilities. This adds quality.
Remark: Personally, I believe Java Spring has solved this very well: The dependency is configured at the consumption (using annotations). This is definitely entperise grade, but I consider it over engineered for the apps

Proposal

I do believe in method signatures of interfaces defining the scope of a method:

executePreMessageSentPrevent(message: IMessage, read: IRead, http: IHttp, persistence: IPersistence): boolean; clearly reads what you are expected to do in this method, which data you get in order to achieve this and which external interfaces you may access. It also lets you know what you can’t do here: manipulate the state of the core objects (no IModify).

But why is IPersistence not available everywhere? Well, since this is a state manipulation. However, once could argue that they both don’t modify the state where it hurts: IHttp may hit the state of a remote system, IPersistence “only” allows changes to the state of the app - which should know by itself how it works.

If you want to simplify without scrutinizing too much of the framework’s purpose, make the accessors which don’t modify the state of the core application globally available by constructor injecting them.
IModify and IExtend however should never be globally available, since you’ll be losing control which you cannot re-gain - and you’re risking issue which are impossible to debug.

4

@ojaegle I was reading your concerns and I realized that some methods may run multiple times cuz they run for each instance or Rocket.Chat that are running in the cluster and may generate unexpected behaviors.

So I asked @douglas.gubert to remove the modifiers, can you check this Pull Request? https://github.com/RocketChat/Rocket.Chat.Apps-engine/pull/100

5

If my explanations helped to discover this, it was worth writing so many words during the past 24hrs :ok_hand:
Now, the result looks at least less dangerous :wink:

There are some aspects I don’t fully understand about the accessors, but seriously, I am currently not able to contribute at this point in time: My knowledge about the framework is outdated. You are the one in-the-know, so: Thanks for taking my earlier concerns seriously - you’ll manage the rest.

1 Like