Improves Controller and Adapter relationship

[flovilmart]

• Controllers that have adapters are AdaptableControllers
• AdaptableController is validating the Adapter type dynamically
• AdapterLoader is responsible for loading the adapter based on mixed params and a defaultAdapter (if provided)

[flovilmart]

@nlutsenko a small refactoring for the Controller / Adapters relationship so we maximise reusabliliy.

Adapters can now be specified in the 'options' as string (to load the module), function to instantiate, or the instantiated object directly.

[drew-gross]

I'd like to end up in a situation where external people can implement adapters without having the parse-server package as a dependency (ideally without having anything at all as a dependency). While this change doesn't prevent that from happening, it doesn't really encourage it either, as our internal adapters now depend on the some abstract base layer and don't provide a good reference for how to implement external adapters without that abstract base layer. I'd actually like to even move the default/internal adapters into their own package and have parse-server depend on them (and them not depend on parse-server) I'm in the middle of implementing mail adapters using this strategy, so I'll have a demo fairly soon.

I'm open to other architectures though. Thoughts?

[flovilmart]

I'm removing the dependency on BaseAdapter, and make a check on the options.constructor !== Object.
and that will help support this transition as fileAdapter: "parse-server-azure-adapter" will be easy to implement.
On the other hand, the inheritance from FilesAdapter is not required, but that also helps

[facebook-github-bot]

@flovilmart updated the pull request.

[facebook-github-bot]

@flovilmart updated the pull request.

[flovilmart]

@drew-gross so that's refactored without the BaseAdapter. There is no need of dependency anymore between adapter and parse-server.

Only the PushAdapter provides super class implementation, to remove code duplication internally.

parse-server should still be a 'dev dependency' and we should be able to run the according tests with 3rd party adapters, this is something that I would have liked to have with the parse-server-urban-airship to ensure the adapter is working correctly as the test suite is exactly the same as the core one.

For the mail adapter, I believe we'll have a MailController, with the same adapter interface,
MailController should conform to AdaptableController, to benefit the common module loading/options mechanics. This way in index.js we'll have a simple pattern:

let mailController = new MailController(mailOptions, MailAdapter);

Mail adapter could this way log in console.warn the calls to email to tell the user that it's improperly configured.

I could also move the Default Adapter into AdaptableController

static setDefaultAdapter(adapter, ControllerClass)

This way we won't externally need to pass it each time we instantiate a controller.

Toughts?

[facebook-github-bot]

@flovilmart updated the pull request.

[flovilmart]

So thanks to ES6 and classes, we have a proper static setDefaultAdapter() on the AdaptableController.
The adapter can be className, object, function, string etc... That's great for supporting 3rd party adapters really, they can be configured from a configuration JSON even with:

{
  push: {
    adapter: "parse-server-urban-airship",
    optionA: "value",
    optionB: "value",
  }
}

That will be the same for mail, and that could even be like:

{
  mail: {
    adapter: {
       send: function(from, to, text, html) {
          // do your mail sending externally.
       }
    }
  }
}

For the adapters, I'll move all of them externally TBH. With this architecture, we'll just have to update for example:

PushController.setDefaultAdapter("parse-server-push");
LoggerController.setDefaultAdapter("parse-server-logger");
FilesController.setDefaultAdapter("parse-server-grid-store-files");

And that for any adaptable controller,

You'll notice too that the index.js don't hold any logic anymore to load the proper adapter. Which make it cleaner :)

[facebook-github-bot]

@flovilmart updated the pull request.

[nlutsenko]

This looks promising, here are my thoughts:

• There is hidden complexity in default adapters if they are designed this way.
  I would personally create a function that can load an adapter from module/function/instance, and pass that at the moment of constructing the controller, which sort of eliminates the whole need for huge portion of AdaptableController class.
  But the most important thing - it doesn't separate out the adapters logic into default/non-default, since we are required to have an adapter at construction time of a controller, which has a guarantee that this thing won't go wrong, and it's easier to track down what is the adapter when we are constructing it.
• Type safety is important. Doing a single AdaptiveController superclass that is responsible for creating/loading/storing an adapter is not type-safe, since adapters have different types, so I would actually recommend against that.
• The next huge piece that looks important to decouple/structure is ClassesController aka what we have right now in rest.js. That baby would actually need FilesController and probably more pieces, so one size fits all won't quite fit.
• Modules for all the things - yes, please. Not feeling strongly whether we should split them into separate packages at this moment, per se, but that we are headed in this direction - absolutely!

So, the only actionable piece here would actually be - restricting the fancy new DefaultAdapter piece into say AdapterLoader.js with functions to purely load the adapter if it doesn't exist and return it of a specific type.

Let me know what you think, I am happy to discuss, improve and restructure this as we go.

I am thinking here in ObjC/Swift flow/structure, since those are my mother-languages and this overall design might feel weird in the world of crazy JavaScript, but I really really hope that the world is moving to this direction, and we can see that with ES6 classes and Flow for type safety.

[flovilmart]

@nlutsenko we could enforce type safe adapters, but that would mean that external adapters have to import the base adapter from parse-server to extend it OR that we compare prototypes with the base adapter. I can do the latter to enforce implementation of required methods.

I can move the adapter loading out of AdapableController into AdapterLoader.
Move the setDefaultAdapert into a setDefaultAdapterForController in AdapterLoader.

AdapterLoader would expose the different loading mechanisms, for all supported types. But still expose a load(options).

The ClassesController decoupling is quite a big thing, would not touch that yet, but focus on the 3 we have decoupled to start having more external contributions for push, files and logs.

[aneeshd16]

@flovilmart

@nlustenko we could enforce type safe adapters, but that would mean that external adapters have to import the base adapter from parse-server to extend it OR that we compare prototypes with the base adapter

What if the base file adapter itself is made into a different repo, and all other file adapters just import that? Something like:

import { FilesAdapter } from 'parse-base-file-adapter';  

export class AzureBlobStorageAdapter extends FilesAdapter { 
.
.

This could solve the problem of type safety. Any new filesasapter would not need to have the entire parse-server as a dependency, just this. Also, i guess this would require minimal changes to other code. What do you think?

[flovilmart]

That would be ideal for sure. That implies a bigger refactoring than what I'm trying to do here :) which is having a common adapter loading mechanism. I'm implementing type-like safety, making sure the adapter has the right methods implemented as well as harder enforcement on loading a controller without an adapter.
@nlutsenko or @drew-gross if you setup the repo, I'm willing to take care of it, but that should not stop this refactor that brings in huge benefits too :)

[facebook-github-bot]

@flovilmart updated the pull request.

[flovilmart]

@nlutsenko The inherent problem with JS compared to Swift is that we're talking 2 completely opposite paradigms: JS prototype based, functional, loosely typed, dynamic language to OO strongly typed static language :) We could, in a near future, type the controllers constructors with Flow, but we have to decide whether we want external adapters to require parse-server or not. Flow Polymorphism is a real Good candidate for that:

class AdaptableController <T> {
constructor(adapter:T) {}
}

class PushController extends AdaptableController<PushAdapter> {

}

I could drop it now, but that would break all 3rd party modules support for the time being...

[drew-gross]

The reason I'm against having external adapters that depend on parse-server is due to versions. I don't want to put people into a situation where they want to update their server, but they are using an adapter that depends on an older version of parse-server. Putting the base adapters into a separate repo doesn't really solve that problem. (I know it's possible to have different dependencies of your own depend on different versions of the same package, but it's not ideal)

My opinion is that aiming for type safety in adapters is "working against the grain of the language" and is not something we should attempt. Rather, at server construction time, we should check the adapters for compliance to the interface. This also allows for easy forward compatibility as we can have optional features in adapters, where you can have the server turn different features on and off based on the functions in the adapter. Scheduled push would be a prime example of this.

[flovilmart]

OK, so as you can see here I'm checking the interface of the adapter: https://github.com/ParsePlatform/parse-server/pull/549/files#diff-51ac4c2ee846910ae669884b9bdc1234R37

A controller has an expectedAdapterType() and we match this type to the passed adapter. This way, optional methods are supported.

[nlutsenko]

Not sure if we need options here, as it looks like it's unused right now.

[flovilmart]

it's definitely unused and the doc is plain wrong!

[nlutsenko]

Agreed with "working against the grain of the language", that's why I referenced ObjC/Swift, because it felt the same way to me.

Looks like this is now inline with our approach towards:

• Don't require parse-server for adapters
• Adapters could be loaded from module/function/class
• Validation happens on the interface of the adapters

Only a single nit in the code, but otherwise LGTM.
@drew-gross, what do you think?

[flovilmart]

@nlutsenko I've hardened the AdaptableController, marked the _adapter private, force the validation upon setter and added a few more tests.

[facebook-github-bot]

@flovilmart updated the pull request.

[nlutsenko]

Lovely! LGTM, merging now.

[drew-gross]

Dang, I had some thoughts but I guess github lost them.

Depending on parse-server is still required for push adapters because there is still a base push adapter.

Having a class with a static function is not really javascript style (for AdapterLoader and load) it's more typical to just have a function (maybe loadAdapter)

[drew-gross]

@flovilmart @nlutsenko

[flovilmart]

Dang...
@drew-gross I initially made it as a class so it' modularized, and maybe we'll want to add more methods there at a point in the future, but I agree that it could be a function.

For the push adapter, it is not required per-se, but I removed some code duplication in our code base. Which is what was intended. Should we move the common logic to the controller?

The only part is classifyInstallations which can be different for each providers.

[drew-gross]

If it can be different for each provider then it should probably be part of the adapter interface, and we should document that it is available/required for implementers to supply it. (and maybe supply a default as a parameter or a separate npm package or something to make life easier for implementers)

[flovilmart]

Ok, so I'll remove the common implementation from the PushAdapter in a separate PR.