Re-thinking Lucid

[thetutlage]

Lucid is one of the largest codebases in AdonisJs core and needs some love now to be more enjoyable and developer friendly.

Consistent output

Lucid has two parts, and both of them returns different outputs for the executed queries.

• Database (query builder) always returns an array of rows.

• Lucid models output varies based on how you are using them.

  1. Calling fetch, paginate, will return Serializer instance.
  2. Ending the query chain without calling fetch, will fallback to Database query builder and returns an array of rows.
  3. Calling find or ending query chain with first, last, will return an instance of the Model and not serializer.

The reasons for different response types are logical. However, it's hard to remember what will be returned when especially if you are a beginner.

Serializer always

Going forward, we will always return an instance of serializer from all queries, no matter if they are made by Lucid models or from Database query builder.

Also, you will have to explicitly end the queries by calling the fetch method. If fetch is not called, the query will not be executed. This solves the issue of forgetting to call the fetch method and getting the response in the wrong format.

Why is serializer required?

Since Lucid is built on top of Active record pattern, we make use of Classes to interact with the database. Which means, the output of the Database can never be plain JSON and we need methods to further interact with the database.

Check this example

const user = await User.find(1)

user.age = 28
await user.save()

If the user was just plain JSON, there is no way to call .save. method on it and execute a query to write to the database.

The shape of data is more tailored for mutations and persisting back to the database. However, when using this data for display inside a view or using for API response, we need to convert its shape, and this is where serializers are helpful.

Serializer API

The serializer will have following API (bare minimum).

serializer.toJSON()

Returns plain JSON back.

serializer.original()

Returns the original output from the Query. It will be plain JSON in case of Database query builder and Model instance, in case of Lucid models.

serializer.isArray()

Returns true, if original is an array. It will be an array when your query can/has returned multiple rows.

serializer.isObject()

Returns true, if original is an object. It will be object, when your intentionally fetches one row from the database. For example: User.find(), User.query().first() and so on.

serializer.size()

Helpful, if you want to know, the query has returned any rows or not.

No to proxies

Those who are not familiar with proxies, it's a way of dynamically resolving properties on an object, without creating or defining those properties.

We used proxies to find query builder methods without defining them on the relationships and the lucid query builder. For example:

When you run the following query, the where method doesn't exist on the hasMany class, we simply proxy is from the Post model query builder.

// posts is a `hasMany` relationship, which returns `hasMany` class instance

user.posts().where('published', true)

This sounds great but very hard to debug. Every part of Lucid uses proxies and trying to find a bug is a nightmare.

We have decided to remove proxies, which does mean that we will have duplicate methods across different classes, but the flow of code will be clear and easier to debug.

Easy to do multi-tenancy

Node.js is async, which is excellent BUT sucks when you want to share HTTP requests data as a global state with other parts of your app.

For example: Telling your models about the currently logged in user. It is impossible to do since two requests can come in parallel and override the global value, leaving you with corrupt state.

Multi-tenancy is all about finding the tenant using the HTTP request data and then making right database queries. To make it easy, we will allow isolated models.

myControllerMethod ({ request }) {
  const tenant = request.header('X-tenant')

  const TenantUser = User.isolated({
    database: tenant
  })

  await TenantUser.all()
}

You can share the output of User.isolated around multiple classes and it will not impact the settings of the actual User model.

Sharing context

This also opens up the possibilities of having computed properties inside your model depend upon the logged in user.

class Post {
  getLikes () {
    return this.ctx.user.id === this.likes.user_id
  }
}

And sharing the ctx as

Post.isolated({
  ctx: { user: auth.user } 
})
.query()
.with('likes')
.fetch()

Database watcher

Another problem with multi-tenant apps is opening too many connections for different databases(one for each client).

Going forward, you can turn on a database watcher, which will close unused connections after x milliseconds.

This will be done using a config property.

{
  connection: {
  },
  watcher: {
    enabled: true,
    closeAfter: '30mins'
  }
}

Sharing parent and root models

When working with relationships, you cannot control the state of the related model, since it's instance is created behind the scenes.

User.with('posts').fetch()

The with method will create an instance of a model defined on the posts relationship. You cannot pass the currently logged in user to that Model since it's out of your control.

However, you can use the $root model property to read the values from the Model, which started the initial query.

const IsolatedUser = User.isolated({
  ctx: auth.user
})

IsolatedUser.query().with('posts').fetch()

And Post model can read values as follows.

class Post extends Model {
  getCreatedByMe () {
    return this.$root.ctx.auth.user === this.author_id
  }
}

Final notes

The shared thoughts will be refined and improved over time. The general idea is to make Models aware of the current HTTP request, which eases the workflow for Multi-tenant apps.

[RomainLanz]

Hey all 👋

Would be cool to wrap the Array of Serializers in a library like collect.js.

That would let us use Collection Methods (like map, filter, reduce, pluck, etc.) directly on the returned collection.

No need to wrap the call in parenthesis and use toJSON().

const documents = (await ...).toJSON()

// Any other Array# methods
documents.map()

const documents = await ...

// Any other collect.js methods
documents.map()

[assertchris]

Calling fetch, paginate, will return Serializer instance.

On board with this whole thing, so long as the serialise must return a model instance when iterated over. What I'd like to see is:

User.where('active', true').first() is the same thing as User.where('active', true).fetch().first().

[EdZava]

It would also be general to add to the Model by configuration that the control of concurrency in the modifications can be made. (It is for when 2 users change the same record)

You could make a simple model like Sequelize does.

[thetutlage]

@EdZava Please don't add generic comments like do X as sequelize does. It simply beats the purpose of me researching and creating proper RFCs.

If you want to help, try to share what can be done with the existing Lucid structure and syntax and don't treat RFC's as your wishlist

[MarlBurroW]

If I got it right, the Model.isolated()method return a copy of Model which will evolve in an isolated way to avoid side effect due to a concurrent request which would alter the model configuration.

So to be safe with multi-tenancy, I guess it will be necessary to use this isolated method in all controllers (and anywhere else), and so manually create an isolated Model everywhere right ?

If I'm right, it could be better to find a way to always have an isolated version when we call a model class to be multi-tenancy ready by default.

[thetutlage]

@MarlBurroW Yes, you got it right.

However, there has to be some entry point in your app, where you define the database or any other model properties for that tenant.

If you don't share that isolated model with your other classes, then you will have to share the tenant with other classes, for them to connect to the right database.

Does it makes sense?

[thetutlage]

@RomainLanz @assertchris Seems like there is still some confusion (which is fine :)).

So let's do a simple quiz and see what you guys expect the output to be from each database query.

1. Query builder fetch

   await Database.table('users').fetch()

2. Query builder first

   await Database.table('users').where('username', 'virk').first()

3. Lucid fetch

   await User.query().fetch()

4. Lucid first

   await User.query().where('username', 'virk').first()

5. Lucid paginate

   await User.query().paginate()

[RomainLanz]

Here are my thinking.

1. Query builder fetch

await Database.table('users').fetch()
// Collection<Serializer>

2. Query builder first

await Database.table('users').where('username', 'virk').first()
// Serializer

3. Lucid fetch

await User.query().fetch()
// Collection<Serializer>

4. Lucid first

await User.query().where('username', 'virk').first()
// Serializer

5. Lucid paginate

await User.query().paginate()
// Collection<Serializer> + Custom Props
// toJSON() outputs something like https://laravel.com/docs/5.7/pagination#converting-results-to-json

[viglucci]

+1 to expected output from @RomainLanz above.

Coming from Laravel I like how all database operations generally return a collection, and then you can operate on them with methods like first() to retrieve the first item in the collection, which is normally useful for when you expect there to only be one result.

[thetutlage]

@RomainLanz You expect await User.query().fetch() to return a collection of serializers, which is not how I meant it to be.

You don't need one serializer per database row, Serializer just wraps all the rows returned by the database.

As per you, this will be the hierarchy

Collection
|
Serializer
|
Model row instance

Whereas, I meant it to be

Serializer
|
Model row instance

[RomainLanz]

True.

Let me update the output.

1. Query builder fetch

await Database.table('users').fetch()
// Collection<Model>

2. Query builder first

await Database.table('users').where('username', 'virk').first()
// Model

3. Lucid fetch

await User.query().fetch()
// Collection<Model>

4. Lucid first

await User.query().where('username', 'virk').first()
// Model

5. Lucid paginate

await User.query().paginate()
// Paginator
// {
//   total: 0,
//   currentPage: 2,
//   perPage: 10,
//   lastPage: 0,
//   data: Collection<Model>
// }

The Collection class will be able to do the same as what Serializer currently do + it will let us use all collect.js functions.

[thetutlage]

In that case I’ll make model do that serializer was doing by defining toJSON method on it.

In that if someone wants to change the output of toJSON method, then they can override the method on the model vs patching the collection class.

[RomainLanz]

Yes, the method .toJSON() should be on the Model, and calling .toJSON() in the Collection will simply do a forEach of each Model it contains and call toJSON() against them.

[MarlBurroW]

The latest @RomainLanz outputs sounds good and straightforward for me.

if someone wants to change the output of toJSON method, then they can override the method

thetutlage

Maybe rather than override the .toJSON() method in the model, it could be more convenient to pass an optional transformer to this method. This would allow to have a flexible and built-in transformation layer.

Example:

const car = await Car.find(1)

car.toJSON() // default JSON output.

car.toJSON('App/Transformers/CarTransformer') // Transformed JSON 
car.toJSON('App/Transformers/CarFullTransformer') // again with another transformer

The transformer could be something like the adonis-bumblebee transformer class and each model could have a default transformer (set inside the model class) to replace the default toJSON() method.

class Car extends Model {
  static get Transformer () {
    return 'App/Transformers/DefaultCarTransformer'
  }
}

And it would work the same with the collections

const cars = await Car.all()

cars.toJSON('App/Transformers/CarTransformer') // forEach and call .toJSON('App/Transformers/CarTransformer') on each item

We can also imagine to pass a transformer to the response

const cars = await Car.all()

response.json(cars).transformer('App/Transformers/CarTransformer')
// or
response.json(cars, 'App/Transformers/CarTransformer')

But it's maybe out of the scope of this RFC, sorry for that.

[mikield]

The last @RomainLanz answer is like Laravel has. But laravel of course seperates Eloquent Colletions from a Support Collection class.

More about Eloquent Collection vs Support Collection.