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

Problem with Typegoose to hide an array of references #99

Open
benjbdev opened this issue Oct 6, 2020 · 12 comments
Open

Problem with Typegoose to hide an array of references #99

benjbdev opened this issue Oct 6, 2020 · 12 comments

Comments

@benjbdev
Copy link

benjbdev commented Oct 6, 2020

Hi !

Thanks a lot for your package, we are using it a lot in our company :)

I have a small issue, may not be related to your plugin, but It could help if you can check this issue on Typegoose :

typegoose/typegoose#397

I'm not able to hide an array of reference.

Thanks for your time

@mblarsen
Copy link
Owner

mblarsen commented Oct 6, 2020

Hi Benjamin, I read through the issue on typegoose and I'm not sure what the conclusion was there.

At any rate would you mind creating a failing test for the array of refs in a PR in this file?

@benjbdev
Copy link
Author

benjbdev commented Oct 6, 2020

Hey, thanks for your response, there is 2 tests case here :

https://github.com/benjaminb-/mongoose-hidden/blob/master/test/github-issues.js

tags should be hidden with hide options set out of type option is working

tags: { type: [TagSchema], hide: true }

the other one tags should be hidden with hide options set in of type option is not passing as the schema configuration is different, the hide option is embedded in the array (Typegoose is building the options that way)

tags: [{ type: TagSchema, hide: true }]

@hasezoey
Copy link

hasezoey commented Oct 6, 2020

hi @mblarsen, where i was reviewing that issue in typegoose, i couldnt find any example (in documentation and tests) about an reference-array: roles: { type: [{ ref:'Role', hide:true }] }, so could you explain where the hide option would need to be? (in-array or out-array)
also:

  • in-array: roles: { type: [{ ref:'Role', hide:true }] }
  • out-array: roles: { type: [{ ref:'Role' }], hide: true }

and another suggestion in that typegoose-issues was, could you maybe add that option to SchemaTypeOptions so that other "mongoose-wrapper" can automatically map it?

@benjbdev
Copy link
Author

benjbdev commented Oct 6, 2020

@hasezoey based on the test to make this plugin work it should be outside, @mblarsen I made this dirty changes to handle the case where the option is inside the type array. Do you agree with these kind of changes ? I can make a proper PR or do you have a better / cleaner idea to do it ? :)

/**
 * Tests to see if the hide property is set on the schema
 *
 * @access private
 *
 * @param {Schema} schema a mongoose schema
 * @param {string} key the key to test
 * @param {Object} doc original document
 * @param {Object} transformed transformed document
 * @returns {Boolen} true of pathname should be hidden
 */
function testSchema(schema, key, doc, transformed) {
  if (typeof schema === "undefined") {
    return false;
  }

  // handle case where options are inside the type array
  const test =
    schema.options.type &&
    Array.isArray(schema.options.type) &&
    schema.options.type.find(opt => opt[key]);

  return (
    schema.options[key] === true ||
    test ||
    (typeof schema.options[key] === "function" &&
      schema.options[key](doc, transformed))
  );
}

@hasezoey
Copy link

hasezoey commented Oct 6, 2020

@benjaminb- does it also handle more than one array?
example: roles: { type: [[[{ type: String }]]] } (3 dimensions)

@benjbdev
Copy link
Author

benjbdev commented Oct 6, 2020

@hasezoey with mongoose :

// this is working
 tags2: { type: [[[{ type: TagSchema }]]], hide: true }

but your case would be converted like that ? correct ?

 tags2: [[[{ type: TagSchema, hide: true }]]]

if yes, the fix I did is not working for that last case, will have to do something else,

maybe we can see the problem in the other way and generate the schema in Typegoose and put the options outside ? Don't know guys, if I can help, tell me :)

@hasezoey
Copy link

hasezoey commented Oct 6, 2020

@benjaminb- that is why i wanted to know where to put that option (and to add it to SchemaTypeOptions for automatic mapping)

@mblarsen
Copy link
Owner

mblarsen commented Oct 6, 2020

@benjaminb- if both options are valid in Mongoose we should probably support them both. Semantically they different too, except for the case where you are not nesting.

@benjbdev
Copy link
Author

benjbdev commented Oct 6, 2020

@mblarsen yes, so are you ok for me to try to implement, including a kind of recursion for the multi dimensions array too, following this exemple ? I'll made the PR,

function testSchema(schema, key, doc, transformed) {
  if (typeof schema === "undefined") {
    return false;
  }

  // handle case where options are inside the type array
  const test =
    schema.options.type &&
    Array.isArray(schema.options.type) &&
    schema.options.type.find(opt => opt[key]);

  return (
    schema.options[key] === true ||
    test ||
    (typeof schema.options[key] === "function" &&
      schema.options[key](doc, transformed))
  );
}

or do you prefer to PR Typegoose and move the options where it should be to handle mongoose-hidden ? What do you think @hasezoey ?

Waiting for your opinion guys don't want to do useless PR :)

@hasezoey
Copy link

hasezoey commented Oct 6, 2020

@benjaminb- i dont know if this plugin handles reference-arrays because, like i said, i didnt find any documentation or something in the tests
and i would be good with either, as long as it gets added to the appropriate SchemaTypeOptions

@mblarsen
Copy link
Owner

mblarsen commented Oct 7, 2020

@benjaminb- yes, please try to make a PR. Once the code is there I can try to help out in terms of how it fits in with everything else.

could you maybe add that option to SchemaTypeOptions

@hasezoey I'm not sure what you meant by this comment. I'm not sure I'll get a PR accepted for a plug-in specific property. Or am I misunderstanding?

@hasezoey
Copy link

hasezoey commented Oct 7, 2020

@mblarsen you could just import mongoose into the plugin and execute:

Object.defineProperty(mongoose.SchemaTypeOptions, 'hidden', Object.freeze({
  enumerable: true,
  configurable: true,
  writable: true,
  value: void 0
}))

(note: SchemaTypeOptions is the class that all other SchemaTypeOptions extend from)
like in the linked code, it would then be available to all that also import mongoose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants