Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mongoose Discriminator - ObjectId / Array type #10130

Closed
redgumnfp opened this issue Apr 13, 2021 · 5 comments
Closed

Mongoose Discriminator - ObjectId / Array type #10130

redgumnfp opened this issue Apr 13, 2021 · 5 comments
Assignees
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@redgumnfp
Copy link

Do you want to request a feature or report a bug?
Bug that's possibly a feature.

What is the current behavior?
A discriminator value is omitted when not a string.

Why is this a problem?
This is frustrating, as I'm unable to use any query operations such as $in, or an existing ObjectId.

I'm unsure why this is tied to only be a string, as it actually works well with either of the non-string values such as:
{$in: array}
or
ObjectId('xyz')

What is the expected behavior?
To be able to use non-string discriminators.

What are the versions of Node.js, Mongoose and MongoDB you are using?
Node v14.16.0
Mongoose v5.12.1
MongoDb v4.4.4

Code specific comment
I have identified the specific line of code which is causing the issue:
mongoose/lib/helpers/model/discriminator line 65.

let value = name;
  if (typeof tiedValue == 'string' && tiedValue.length) {
    value = tiedValue;
   }

This can be updated to (or the logic omitted entirely):

let value = name;
value = tiedValue;

Other comments
Only thought is there's some sort of security concern?

@IslandRhythms IslandRhythms added the discussion If you have any thoughts or comments on this issue, please share them! label Apr 14, 2021
@redgumnfp
Copy link
Author

Similarly, today, I was attempting to get this to work the other way around.

const mongooseSchema = new mongoose.Schema({
  tenantRefs: [{
    type: String,
    unique: false,
    required: true,
    index: true,
  }],
  otherData: {
    type: String,
    unique: false,
    required: true,
    index: true,
  },
  ...  
});

This can then be consumed with a dynamic model, created at run-time:

const tenantWrapper = (name, schema, options) => () => {
  // pull these from CLS, or pass in manually
  const tenantValue = getTenantValue();
  const skipTenant = getSkipTenant() || false;

  const Model = connectionRegional.model(name, schema);
  if (skipTenant) {
    Model.schema.set('discriminatorKey', null);
    return Model;
  }


  const discriminatorName = `${name}-${tenantValue}`;
  schema.set('discriminatorKey', 'tenantRefs');

  const existingDiscriminator = (Model.discriminators || {})[discriminatorName];
  if (existingDiscriminator) return existingDiscriminator;


  return Model.discriminator(
    discriminatorName,
    new mongoose.Schema({}, options),
    tenantValue,
  );
};

In essence, I'm attempting to utilise the tenantRefs array to automatically filter depending on the user's tenantRef. If you can imagine a marketplace scenario, there's an array including a supplier ID and a client ID.

This works perfectly on all query/aggregation methods, but frustratingly, not when creating a document.

Due to this snippet in /lib/document.js, the array is split out:

    if (this.$__schema.singleNestedPaths[path] == null) {
      // If this path is underneath a single nested schema, we'll call the setter
      // later in `$__set()` because we don't take `_doc` when we iterate through
      // a single nested doc. That's to make sure we get the correct context.
      // Otherwise we would double-call the setter, see gh-7196.
      val = schema.applySetters(val, this, false, priorVal);
    }

This:
['123456', 'ABC123']
Becomes this:
[['123456'], ['ABC123']]

Temporary work around
As a work-around, I'm using upfront query validation to ensure that isolation fully exists, but switching the discriminator off for document creation.

@vkarpov15 vkarpov15 removed the discussion If you have any thoughts or comments on this issue, please share them! label Apr 19, 2021
@vkarpov15 vkarpov15 added this to the 5.12.6 milestone Apr 19, 2021
@vkarpov15 vkarpov15 modified the milestones: 5.12.6, 5.12.7 Apr 27, 2021
@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Apr 28, 2021
@vkarpov15
Copy link
Collaborator

The tricky part is checking whether discriminator values are equal, so it is unlikely we'll ever allow a pojo as a discriminator tied value for example. But we should be able to easily support ObjectIds and numbers as tied values. Is there some other type you need?

@redgumnfp
Copy link
Author

Great that you've added ObjectIds to the scope @vkarpov15 - helps to reduce duplicated fields in MongoDb, thank you! 🙌

An array of values (string/objectId/number) would be ideal if no pojo, but imagine that's a fair amount of duplicated code on your end, and it would be unclear if it's "all" or "in" for the comparison without additional configuration options.

@vkarpov15
Copy link
Collaborator

Array of values will be tricky, because comparing arrays in JavaScript is complex in general. What's your use case for using an array as a tiedValue?

@redgumnfp
Copy link
Author

Apologies for the delay.

I utilise discriminators for schema-based tenant isolation.

Cross-functional team example
Each team works on X projects.
Each team has Y users.
A user can work within multiple teams.

To find the teams that a user has, the userId could be within a "users" array field on the Team collection - this works fine.
To find the projects a team has, the teamId could be within a "teams" array field on the Project collection - this works fine.
To find the projects that a user has, the teamIds (plural) could be within a "teams" array field on the project collection too, however it's not possible to pass in the array of teamIds, only one teamId.

This can be gotten around with a users field on the project collection too, however it could grow significantly over time and requires a separate logic condition to update projects when adding users to teams.

I'd say it's "nice to have" an an option. I'm considering writing a separate plugin to act as middleware on all methods in the future; I think using discriminators is good, but not quite what it was built for.

The enhancement of being able to use Numbers and ObjectIds is a fantastic result anyway, so thank you.

FYI - I'll copy my second comment into a new issue, as it appears to have been missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

3 participants