Objection.js: POLL: Should we add an alternative object syntax for relation expressions?

[koskimas]

Currently all relation expressions are strings. Should we add an alternative object representation for them?

Reasons to add something like this:

1. object expressions would be easier to create and inspect programmatically
2. Simple array expressions are just an array of relation names, which is intuitive and clean.

Reasons to not add something like this:

1. Multiple ways to do the same thing.
2. Aliases and filters are difficult to add while keeping the expressions simple.

Here's some examples how this could work:

Simple relation expressions

// All following are equivalent:

// string
const e1 = `[a, b.c]`;

// array of strings
const e2 = ['a', 'b.c'];

// all objects
const e3 = {
  a: {},
  b: {
    c: {}
  }
}

// all objects 2
const e4 = {
  a: true,
  b: {
    c: true
  }
}

// combination of objects and arrays
const e5 = {
  a: {},
  b: ['c']
}

Expressions with aliases and filters

// All following are equivalent:

// string
const e1 = `[
  a(f1) as aa, 
  b(f2).c(f3, f4)
]`;

// array of strings
const e2 = [
  'a(f1) as aa', 
  'b(f2).c(f3, f4)'
];

// all objects (ugly as hell)
const e3 = {
  'a(f1) as aa': true,
  'b(f2)': {
    'c(f3, f4)': true
  }
}

// combination of objects and arrays  (ugly as hell)
const e4 = {
  'a(f1) as aa': true,
  'b(f2)': ['c(f3, f4)']
}

// all objects, filters and aliases using special properties:
const e5 = {
  a: {
    $alias: 'aa',
    $filter: ['f1']
  },

  b: {
    $filter: ['f2']

    c: {
      $filter: ['f3', 'f4']
    }
  }
}

Alternative verbose syntax

// All following are equivalent:

// string
const e1 = `[
  a(f1) as aa, 
  b(f2).c(f3, f4)
]`;

const e2 = [
  {
    name: 'a',
    filter: 'aa'
  },
  {
    name: 'b'
    filter: ['f2'],

    children: [
      {
        name: 'c',
        filter: ['f3', 'f4']
      }
    ]
  }
]

So what do you think? Should this be added and with which syntax? If you have an alternative syntax in mind, I'd love to hear it.

[elhigu]

I generally like the idea that there is also format that is not plain string based 👍 I don't have any additional ideas for syntax.

[koskimas]

@jadengore Sorry to pull you into this issue like this, but I read you blog (thanks for the kind words by the way ❤️) and I wondered if you'd have any thoughts about this issue? You're not the biggest fan of the current relation expressions if I understood this correctly

While I’m not a huge fan of the “stringified arrays”, I am willing to forgive this since the API is truly in a class of its own.

[devinivy]

I have also seen folks trip on the array-looking syntax. I personally like it, and ultimately do not have a need/desire for any changes like this, especially with the ability to do things like mergeEager() and allowEager(). That said, I can see the value of an equivalence between [ a, b ] and ['a', 'b']– even though the confusion can probably be solved somewhat via documentation.

[lehni]

I am interested in a way for the QueryBuilder to be able to provide its current eager expression, and allow the user to further alter it programmatically.

So if the relation expressions could be provided in either form above, and be accessed through builder.eagerExpression in a defined form, that'd be of interest to me.

I think both the 'all objects, filters and aliases using special properties' and 'alternative verbose syntax' are both contenders for me for a standardised format returned by such a builder.eagerExpression getter.

Alternatively, it could return the RelationExpression object itself, if that was made part of the public API and would offer ways to programatically iterate and modify the expressions further, or be used to build new ones, if they were to be seen as read-only.

I've been tinkering with this currently, and find the RelationExpression class very clear and easy to work with in its current form. It could benefit from some added methods, e.g. addFilter() which would check for redundancy in filters and only add if it wasn't already there, hasFilter(), etc.

[lehni]

But coming back to the above proposals for inputting expressions: All things considered, I would probably still use the string notation as it is the shortest, most concise version. The only issue is maybe with root-level pseudo-arrays, which could be supported by RelationExpression.create() accepting an array of strings also.

[koskimas]

The only issue is maybe with root-level pseudo-arrays, which could be supported by RelationExpression.create() accepting an array of strings also.

That may be the biggest source of confusion and would be trivial to add.

[lehni]

Yes and it wouldn't require you to reinvent the wheel...

[lehni]

Also, in UpsertGraphOptions and InsertGraphOptions, you already support arrays of relation expressions.

[koskimas]

Actually those are not relation expressions, but a list of dot separated paths. There one needs to be able to say both

• foo.bar but not foo
• foo.bar and also foo

Using a relation expression that's not possible. [foo, foo.bar] is not a valid relation expression. That's why there's a list of them. And they are not real relation expressions because I didn't want to further complicate the monstrosity that is the upsertGraph algorithm.

(threads would be a nice feature for github chat. We are polluting this issue with this stuff)

[mceachen]

Just from an API standpoint, I'd like to see just one way of doing things. In the past I've gotten in trouble with "trying to be nice" with multiple overloaded signatures, and it causing more confusion than introducing comfortable ergonomics.

If the object-based query is more performant and easier to reason from an implementation standpoint, I'd be fine with the API break (esp before 1.0).

[elhigu]

@mceachen I agree that usually having just one way is the best solution.

However in this case I would see this object based format as kind of "real format" for representing these things in objection and user friendly DSL strings are just for humans to have more understandable presentation of the same thing.

Having only "the real object" format without that optional DSL layer would be pretty inconvenient for average human users. And for machines having to produce DSL strings for autogenerated code would be likewise inconvenient.

[koskimas]

Yeah, we are not going to remove the old string DSL so there should be no breaking API changes even if we add this. This doesn't need to happen before 1.0.

[benschenker]

After using Objection for some time I agree that the eager strings are problematic. I'm still not really sure of the use case letting the client send an eager string so it kind of seems unnecessary. I do appreciate the need to not have a breaking change here though. See below what I came up with to solve the problem of merging eager strings which is working now in production. I definitely think that an alternative syntax would be a good idea and possibly less confusing.

I ran into a use case where I needed to be able to merge eager strings together so I came up with my own dsl with an eager object that looks like:

{
  children: {
    pets: {},
    movies: {},
  },
}

which would turn into an eager string like: '[children.[pets, movies]]'

A small recursive function that converts from my eagerObject to a standard eager string which just needs to be surrounded by [] (_ is lodash here):

const eagerObjectToEagerString = eagerObject =>
  _.map(
    eagerObject,
    (val, key) =>
      _.isEmpty(val) ? key : `${key}.[${eagerObjectToEagerString(val).join(',')}]`
  );

Nice thing here is that you can then use _.merge (or something similar) to combine different eagerObjects and then convert it to the eager string.

[koskimas]

@benschenker There is the mergeEager method.

[koskimas]

But yeah, I think the cleanest DSL would be the all objects approach @benschenker used. Filters and aliases could be specified using special properties prefixed by $.

{
  children: {
    pets: { $modify: ['onlyDogs'] },
    movies: {},
  },
}

I suppose inline filters could be supported too:

{
  children: {
    pets: { $modify: qb => qb.where('spcies', 'dog') },
    movies: {},
  },
}

[lehni]

Looks good to me. So $modify would support both named filters and filter functions?

[lehni]

Optionally, you could allow true as a replacement for {} for more explicit code. But it would complicate the parser a little, and make the syntax longer:

{
  children: {
    pets: { $modify: ['onlyDogs'] },
    movies: true,
  },
}

But this may be more readable and explicit, which is maybe a plus?

[lehni]

One last thought: If such a syntax is added, I think it would be best if it only supported one way of doing things, not optional variations. Then, all that could be exposed in the public API is a parser method that converts any supported form of such expressions to this native JS form (Pojos).

That way, plugins that want to mingle and extend expressions can rely on this parser and then traverse and modify the Pojo structure without the need for any additional classes or methods from Objection, and pass the result back to eager() / mergeEager() & co.

Perhaps Objection could even be changed internally to work with these Pojo expressions instead of the RelationExpression in the processing of these expressions.

[koskimas]

Perhaps Objection could even be changed internally to work with these Pojo expressions instead of the RelationExpression in the processing of these expressions.

Nope.

[koskimas]

Actually the "all objects" format doesn't work. There is no way to give same relation two aliases. For example:

{
  children: {
    pets: { 
      $modify: ['onlyDogs'],
      $alias: 'dogs',
    },
    pets: { 
     $modify: ['onlyCats'],
     $alias: 'cats'
    }
  },
}

[koskimas]

The alias feature narrows our options to verbose AST-style DSLs like:

  {
    name: 'children',

    // This property's name could be `childNodes` or something like that.
    // Not to be confused with the name of this relation in this example.
    children: [
      {
        name: 'pets',
        alias: 'dogs',
        modify: ['onlyDogs']
      },
      {
        name: 'pets',
        alias: 'cats',
        modify: ['onlyCats']
      }
    ]
  }
}

[lehni]

You could extend the object-only syntax a bit, by allowing arrays as well as objects for relations, and then it could be done:

js { children: { pets: [{ $modify: ['onlyDogs'], $alias: 'dogs', }, { $modify: ['onlyCats'], $alias: 'cats' }] }, }

[lehni]

Another option could be to reverse the logic of the aliasing:

{
  children: {
    dogs: { 
      $relation: 'pets',
      $modify: ['onlyDogs']
    },
    cats: { 
     $relation: 'pets',
     $modify: ['onlyCats']
    }
  },
}

[koskimas]

Another option could be to reverse the logic of the aliasing

That's a good idea!

[ebramanti]

@koskimas Sorry for taking so long to get back to you, work has been crazy.

My feel on it is an object DSL would be most appropriate. A couple reasons for this:

1. Relation mappings use objects for representation of nested relations, so it makes sense to match that for consistency (even though these APIs do serve somewhat different functions).
2. Using an object API can make error reporting more robust, since its easier to point out the nesting of eager relations.

I also think making this a breaking change for 1.0 is fine, and agree with @lehni that there should be one way of doing this (no longer supporting string representation).

[lehni]

I wasn't actually trying to say that... I think the string representation can stay.

[ebramanti]

@lehni my bad... Like I said in my blog, not a deal breaker. Would be cool to be able to enforce an AST using TS or something.

[koskimas]

I ended up adding the object representation suggested by @lehni

{
  children: {
    dogs: { 
      $relation: 'pets',
      $modify: ['onlyDogs']
    },
    cats: { 
     $relation: 'pets',
     $modify: ['onlyCats']
    }
  },
}

Basic queries look like this:

Person
  .query()
  .eager({
    pets: true, // or {},
    movies: {
      actors: true
    }
  })

aliases:

Person
  .query()
  .eager({
    dogs: {
      $relation: 'pets',
      $modify: ['onlyDogs']
    }
    movies: {
      actors: true
    }
  })

recursions:

Person
  .query()
  .eager({
    parent: {
      $recursive: 5
    }
  })

This is now also the internal format used by RelationExpression class, so the codebase did not get any more complex.

The old string expressions still work. There's no documentation written yet. I'll release this with 1.1.0 along with the documentation.

[lehni]

Very cool! Does $filter also work instead of $modify? Other parts of the code allow the use of both terms...

[koskimas]

$modify was added later to other parts, and I've been meaning to deprecate filter everywhere. No need to have multiple names for the same thing. modify is better than filter since not all functions are filters.

[lehni]

Oh I see. I just thought it makes sense in conjunction with name filters...

[koskimas]

Yeah, I regret that name. It should have been namedModifiers

[lehni]

I still think scopes would have been a great name for them! It's what other ORMs call that concept, e.g. http://guides.rubyonrails.org/active_record_querying.html#scopes

[lehni]

Also, you could still rename them to namedModifiers, and keep supporting both for the time being.

[lehni]

Or just modifiers: {}, while you're at it? It's kind of obvious that they're named :)