Changing how we deal with resources #118

einari · 2021-02-21T10:44:01Z

Summary

This pull request is a major change.
The biggest change is the removal of TypeGoose and Mongoose as default - instead it leverages now the native MongoDB driver and all setup is configured for this.

Added

Resource system for getting resource configurations per type per tenant (Add support for resource.json for hooking up the right connection to readmodels #111)
Introducing an unobtrusive way to work with MongoDB driver and still get the benefits of custom serialization with custom types.
Context object configured through middleware containing the tenant identifier, user id and cookies. (Automatically setup execution context based on the tenant information in web requests #112)

Changed

Changing from TypeGoose to native MongoDB driver for NodeJS and configured it by default
TenantId on Context is now the type TenantId from @dolittle/sdk.execution

Removed

TypeGoose and Mongoose are both moved as dependency and hence not configured by default as a consequence
Environment variables for configuring database and event store access

…into main

…cally deserializes to correct type

pavsaund

Yowza! that was quite a ride 😄. I learned a lot by looking at this PR and spent quite a lot of time with Google opened as I went 😅 . I've left a trail of comments, mostly for observation and some questions. There a few suggestions a well, but none of these is blocking.

I tested with the SampleApp, and that worked nicely. What I didn't get to test is with an existing Vanir code-base. How will an existing codebase update its MongoDB usage? Will all queries need to be re-written, or can those projects continue with TypeGoose / Mongoose if they desire? I assume that they can, but I haven't understood how. Could you write a few words somewhere about that, or point me to those words if I've missed it?

pavsaund · 2021-02-23T14:20:18Z

Source/typescript/backend/resources/ResourceConfigurations.ts

+    private ThrowIfMissingResourceConfigurationOfType<TResource extends ResourceConfiguration>(resourceConfigurations: Map<Constructor<{}>, any> | undefined, resourceConfigurationType: Constructor<TResource>, tenantId: TenantId) {
+        if (!resourceConfigurations || !resourceConfigurations.has(resourceConfigurationType)) {
+            throw new MissingResourceConfigurationOfType(tenantId, resourceConfigurationType);
+        }
+    }
+
+    private ThrowIfMissingResourceConfigurationsForTenant(tenantId: TenantId) {
+        if (!this._resourceConfigurationsByTenants.has(tenantId.toString())) {
+            throw new MissingResourceConfigurationsForTenant(tenantId);
+        }
+    }
+}
+


seeing these methods are private, should they not be pascalCased?

Indeed they should :) - and not just because they’re private since thats what we do for all properties and methods & functions. A bit surprised we don’t have a lint rule set for this.

pavsaund · 2021-02-25T09:39:05Z

Documentation/backend/typescript/mongodb.md

+The serialization subsystem introduces the concept of a `CustomType` - these are the ones used to convert to and from
+MongoDBs BSON binary.
+
+Creating your own custom type is easy, all you need to do is to implement the abstract class `CustomType`:


This section is good to have and read about. A question comes to mind (sorry if the answer is obvious): When would one need to create a custome type? For enum's, array's, every complex object to be stored? A little clarification here may be worth adding

Great question - I think the documentation could be clearer on this - I’ll add more clarity around it.
CustomTypes are only for those occassions where you want it stored in a particular way rather than what the default behavior is. If you take the Guid that we have a custom type for, if we didn’t have the custom type - it would by default recursively serialize the class Guid and end up with all the bytes as a byte array plus any other properties on that type. While what we’re looking for for that particular type is a way to be able to query it easily in Mongo and also easily read it (or more easily - Guids are hard to read anyways :)).

A good example of a CustomType we probably would like to have is for the DateTime type we’ve been using. Serializing it by default would end up with a structure like this being saved:

{ «year»: 2021, «month»: 2, «day»: 1, «hour»: 13, «minute»: 37, «seconds»: 37, «milliseconds»: 654 }

While what would be nice is that it ended up like the date type in MongoDB.

I’ve added an issue for the DateTime type : #119

I think for the most part CustomTypes aren’t needed.

Hope that made sense.

pavsaund · 2021-02-25T09:55:37Z

Documentation/backend/typescript/resources.md

+These resources are managed for you in the [Dolittle platform](https://dolittle.io/docs/platform/requirements/#1-your-application-must-use-the-resource-system).
+And its therefor important that solutions support this. Vanir brings a resource system


This page in the platform documentation doesn't actually describe what the Dolittle platform defines as resources.

Ex: can we introduce custom resource configurations here that can be used in the app, or are these specific platform resources that are well-defined?

Great question. The platform documentation leaves us the opportunity to change this - as we have plans to improve on how we do things. Today the solution is the resources.json file and it is part of the deployment yaml file - which is not available for modification to anyone outside Dolittle. It is limited to basically EventStore and MongoDB for ReadModels as that are the managed resources we have.

I can add a bit more details on this in this documentation.

pavsaund · 2021-02-25T12:21:38Z

Samples/Source/Typescript/Backend/MyController.ts

+    @Get()
+    async doStuff(): Promise<MyType[]> {
+        const collection = await this._db.collection(MyType);
+        await collection.insertOne({
+            _id: Guid.create(),
+            something: 42
+        });
+        const result = await collection.find().toArray();
+        return result;
+    }


This returns the following result to the frontend over Swagger. Is this right?

{ "something": 42, "_id": { "bytes": [ 14, 103, 48, 90, 28, 148, 77, 210, 163, 30, 9, 218, 40, 226, 118, 28 ], "_stringVersion": "5a30670e-941c-d24d-a31e-09da28e2761c" } },

That is expected behavior. Thats how a Guid type looks like. When using GraphQL we have in Vanir a scalar type (https://github.com/dolittle-entropy/vanir/blob/main/Source/typescript/backend/data/GuidScalar.ts) that converts it back and forth. This way we get to use the Guid type for Guids in code, have it converted using our CustomType construct for Mongo.

I haven’t looked into TSOA and if its possible to have a similar experience for ReST APIs.
Adding an issue to investigate it (#120)

pavsaund · 2021-02-25T12:37:16Z

Source/typescript/backend/DefaultConfiguration.ts

-    database: {
-        host: 'localhost',
-        database: 'data',
-        port: 27017
-    },
-    eventstore: {
-        host: 'localhost',
-        database: 'events',
-        port: 27017
-    },


Shouldn't the vanir.json file also remove the database and eventstore properties? Both for the Sample, and the templates in create-dolittle-microservice.

Good catch - indeed it should :)

pavsaund · 2021-02-25T13:21:41Z

Source/typescript/backend/mongodb/MongoDatabase.ts

+        if (options) {
+            return db.collection(name, options);
+        }
+        const collection = db.collection(name);


can this be simplified to sending in default empty options. Something like this (can also be set as default value in method parameter):

const collection = db.collection(name,options || {});

I don't know if this actually changes the behaviour though.

On second look of the context of the code - this would change the behavior. This is the whole dance of overloaded methods that creates for a bit of messy implementations. So the condition is basically for a specific overload.

pavsaund · 2021-02-25T22:35:11Z

Source/typescript/backend/mongodb/MongoDbReadModelsConfiguration.ts

+// Copyright (c) Dolittle. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+import { ResourceConfiguration } from '../resources';


Shouldn't the MongoDbReadModelsConfiguration be in the resources folder next to EventStoreConfiguration? Or should the EventStoreConfiguration be moved to another folder, perhaps?

I think its more the latter. The reason is cohesion - keep all the MongoDb stuff in one place. We just don’t have any EventStore things yet. But I think a better place for the EventStoreConfiguration would be the Dolittle folder. I’ll move that to there.

Yes, that makes more sense! 👍

pavsaund · 2021-02-25T22:40:45Z

Source/typescript/backend/resources/ResourceConfigurations.ts

+                this._resourceConfigurationsByTenants.set(tenantId.toString(), resourceConfigurationsMap);
+
+                for (const resourceType in tenantResources) {
+                    switch (resourceType) {


I guess this answers my question earlier about which resource types the platform supports :D. Should this become a string-enum with well-known things?

pavsaund · 2021-02-25T22:45:23Z

Source/typescript/backend/web/Context.ts

+import { TenantId } from '@dolittle/sdk.execution';
+
+const ContextKey = 'Context';
+const ns = cls.createNamespace('0e852ce9-ef09-45d2-b64b-4868de226563');


this is really interesting. It functions as a request cache, right? Is the Guid is just a random guid you put there, or does it connect with something else?

I see it's also used in the mongo db context middleware - didn't notice it before. Nice

The key can be anything - as long as its unique.

What it does is give us a key/value store per async context - making it safe to store values and be sure you’re using it in the right context when you’re in the same call context. Similar to AsyncLocal in C#/.net.

Our use case is to be able for each web request coming in to set the correct context based on TenantId, UserId coming in and such and make this accessible in a good way throughout different parts of the request.

Its leveraging a project called cls-hooked for this - which makes it a bit nicer to do. You can read more on it here: https://medium.com/trabe/continuation-local-storage-for-easy-context-passing-in-node-js-2461c2120284

That looks like a nice and unobtrusive way of adding this context information. I read the blog post and it links to some performance issues that may be worth considering, as this is adding a significant performance hit according to that Github thread. nodejs/benchmarking#181

You’d also want to stay away from async_hooks if you’re writing a library since you could impact performance for your users just for your convenience.

There doesn't seem to be any improvement or resolution though. Perhaps another approach may be better for this, or we could see how it actually impacts applications? Perhaps creating a follow-up task for this?

Good to keep an eye on the performance. I've added an issue (#121) as you suggested.

Great catch! The benchmarks they had there was on version 9 of Node. I think for most of our current scenarios we are ok.

https://github.com/bmeurer/async-hooks-performance-impact

It talks about # of requests a second in the thousands.

pavsaund · 2021-02-25T22:47:17Z

Source/typescript/backend/web/Express.ts

+    app.use(ContextMiddleware);
+    app.use(MongoDbContextMiddleware);
+


Ah, tying it all together!

einari · 2021-02-26T06:05:40Z

Great feedback and questions. I’ll amend the documentation a bit and also fix the things you pointed out in the code. Thanks!

einari · 2021-02-26T09:57:01Z

To answer your questions related to what we do with solutions using this.
My suggestion is to stop using Mongoose. The reason I suggest it is that from what I've been seeing thus far is that we aren't really using Mongoose for. It brings to the table a couple of things; schemas and validation.
Both of these are great and potentially very useful things. The validation part is something we've started implementing towards GraphQL instead (https://app.asana.com/0/1199900680512299/1199916836633356).

That leaves us with the schema part. There are some aspects to it that is interesting.
But if we look at what we're working on these days related to projections - the versioning of schemas and maintaining the schemas becomes a bit obsolete in that regard (https://github.com/dolittle-entropy/projections).

That being said, please come with other perspectives I'm not seeing and if there is something missing in this assumption.

einari added 16 commits February 19, 2021 06:33

Fixing proxy path for graphql

8101235

Fixing ports

666c29f

Merge branch 'main' of github.com:dolittle-entropy/applications-node …

1f7ba31

…into main

Started setting up the basics of the resource configuration system

83dce9e

Resource system ready for use

ff38bbc

Introducing support for custom types for mongodb driver

c2f08a2

Started transitioning away from typegoose/mongoose

92affc7

Up and running with multi tenanted MongoClient + MongoDb

eca0dfa

Adding extensions the make it unobtrusive to use MongoDB and automati…

e65709a

…cally deserializes to correct type

Adding JSDocs

1a8cb6e

Making it possible to take a dependency to configuration directly

7456c7e

Updating env variables documentation

0761d08

Adding default resources.json file

5c164d2

Adding some basics for documentation

f3f5a43

Updating anatomy docs

3aa7264

Fixing anatomy docs

3812bb3

einari added the major label Feb 21, 2021

einari added 5 commits February 21, 2021 11:49

Adding details on context

1cf8378

Setting up projections using the new provider system for configuration

a4bfd46

Moving Dev Dependencies to dependencies

c81550e

Putting in docs on middlewares

483bc5b

Removing whitespace

ade1bec

einari marked this pull request as ready for review February 21, 2021 13:42

einari assigned pavsaund Feb 21, 2021

einari added 3 commits February 21, 2021 14:47

Changing to use the EventStoreConfiguration for the intermediates

f30cd7f

Lint fixes

9989173

Disabling ESLint rules that doesn't make sense in the context

750227f

pavsaund removed their assignment Feb 23, 2021

pavsaund approved these changes Feb 25, 2021

View reviewed changes

einari added 6 commits February 26, 2021 09:43

Fixing casing on methods

280532c

Adding an improved description around custom types as to why

16cc1db

Adding more details on resources and thinking behind it

fadfd48

Fixing Vanir.json files to not have DB/EventStore configs

76daabf

Moving EventStoreConfiguration into Dolittle folder

af78b04

Add information about Cursors and find methods

f275819

Fixing reference to event store configuration

f9787e6

einari merged commit b6a35c9 into main Feb 26, 2021

einari deleted the multi-tenant-resources branch February 26, 2021 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing how we deal with resources #118

Changing how we deal with resources #118

einari commented Feb 21, 2021 •

edited

Loading

pavsaund left a comment

pavsaund Feb 23, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 26, 2021

pavsaund Feb 25, 2021

pavsaund Feb 25, 2021

pavsaund Feb 25, 2021

einari Feb 26, 2021

pavsaund Feb 26, 2021

einari Feb 26, 2021

pavsaund Feb 25, 2021

einari commented Feb 26, 2021

einari commented Feb 26, 2021

		These resources are managed for you in the [Dolittle platform](https://dolittle.io/docs/platform/requirements/#1-your-application-must-use-the-resource-system).
		And its therefor important that solutions support this. Vanir brings a resource system

		app.use(ContextMiddleware);
		app.use(MongoDbContextMiddleware);

Changing how we deal with resources #118

Changing how we deal with resources #118

Conversation

einari commented Feb 21, 2021 • edited Loading

Summary

Added

Changed

Removed

pavsaund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

einari commented Feb 26, 2021

einari commented Feb 26, 2021

einari commented Feb 21, 2021 •

edited

Loading