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

Self-Containing Classes #42

Closed
hasezoey opened this issue Sep 20, 2019 · 25 comments
Closed

Self-Containing Classes #42

hasezoey opened this issue Sep 20, 2019 · 25 comments
Labels
bug Something isn't working cant fix This cant be fixed without external modules being updated has repro script This issue has an reproduce script help wanted Any help would be appreciated

Comments

@hasezoey
Copy link
Member

hasezoey commented Sep 20, 2019

Continuation of szokodiakos/typegoose#19

Versions

  • NodeJS: 12.10.0
  • Typegoose(GIT): 38b9f79

Code Example

class SelfContaining {
  @prop()
  public nest?: SelfContaining;
}

getModelForClass(SelfContaining);

Do you know why it happenes?

Typegoose tries to build the schemas that are a prop


PS: i have absolutely no clue on how to fix this

Edit 1: addition, i didnt just meant like the code example, it was just very basic, here an proper example:

class SelfContaining {
  @prop()
  public nest?: SelfContaining;
}

const selfContainedModel = getModelForClass(SelfContaining);
const doc = new selfContainedModel({ nest: new selfContainedModel({ nest: new selfContainedModel({}) }) });
@hasezoey hasezoey added bug Something isn't working help wanted Any help would be appreciated has repro script This issue has an reproduce script labels Sep 20, 2019
@hasezoey hasezoey changed the title Self-Containing-Class Self-Containing-Classes & Nested-Self-Containing-Classes Nov 1, 2019
@dfseifert
Copy link

I did some testing and the following works for building the model:

// ./src/internal/schema.ts

export function _buildSchema<T, U extends AnyParamConstructor<T>>(
  cl: U,
  sch?: mongoose.Schema,
  opt?: mongoose.SchemaOptions
) {
  // ...
  
  if (!isNullOrUndefined(decorators)) {
    for (const decorator of decorators.values()) {
      _buildPropMetadata(decorator, cl); // pass down class
    }
  }

  // ...
}
// ./src/prop.ts

export function _buildPropMetadata<T, U extends AnyParamConstructor<T>>(input: DecoratedPropertyMetadata, cl: U) {
  // ...
  
  if (utils.isNotDefined(Type)) {
    buildSchema(Type);
  }

  if (cl === Type) {
    return;
  }

  // ...
}

Now the model looks like

// selfContaining.ts
import { prop } from '../../src/prop';
import { getModelForClass } from '../../src/typegoose';

class SelfContaining {
  @prop()
  public bla?: number;

  @prop()
  public nest?: SelfContaining;
}

const SelfContainingModel = getModelForClass(SelfContaining);
export default SelfContainingModel;

And the test looks as following

// selfContaining.test.ts
import { expect } from 'chai';

import SelfContainingModel, { SelfContaining } from '../models/selfContaining';

// Please try to keep this file in sync with ./arrayValidator.test.ts

/**
 * Function to pass into describe
 */
export function suite() {
  it('should not throw exception', (done) => {
    expect(SelfContainingModel.create({
      bla: 5,
      nest: new SelfContaining()
    })).to.eventually.be.fulfilled.and.notify(done);
  });
}

@hasezoey
Copy link
Member Author

hasezoey commented Jan 6, 2020

@lab9 could you make an PR?

@dfseifert
Copy link

dfseifert commented Jan 6, 2020

@lab9 could you make an PR?

Will do so later.

Info:
further testing revealed that the instance i get back is missing the nest element.

it('should not throw exception', async () => {
    const model: SelfContaining = await SelfContainingModel.create({
      bla: 1,
      nest: {
        bla: 2,
        nest: null
      }
    }) as SelfContaining;

    expect(model.bla).to.be.equal(1);
    expect(model.nest!!.bla).to.be.equal(2); // TypeError: Cannot read property 'bla' of undefined
  });

@hasezoey
Copy link
Member Author

hasezoey commented Jan 6, 2020

further testing revealed that the instance i get back is missing the nest element.

thats is exactly what i couldnt figure out, because javascript isnt "recursive" in the meaning of "apply current object 'infinitly' down on this property"
-> that is the reason i decided to make it an hard error, instead of allowing "one-time-nesting" (which would be probably not what the user(/dev) wants)

Will do so later.

because of that, it a pr is not needed anymore

thanks for trying :)

@dfseifert
Copy link

Have a look at the following more or less workaround:
Quite amusing but it works when you use an interface.

import { prop } from '../../src/prop';
import { getModelForClass } from '../../src/typegoose';

interface Folder {
  displayName: string;
  subFolders: Array<Folder>;
}

export class Storage {
  @prop()
  public bla?: number;

  @prop()
  public root: Folder;
}

const StorageModel = getModelForClass(Storage);
export default StorageModel;

The test goes through:

import { expect } from 'chai';

import StorageModel, { Storage } from '../models/storage';

// Please try to keep this file in sync with ./arrayValidator.test.ts

/**
 * Function to pass into describe
 * ->Important: you need to always bind this
 */
export function suite() {
  it('should not throw exception', async () => {
    const storageInstance: Storage = {
      bla: 1,
      root: {
        displayName: 'root',
        subFolders: [
          {
            displayName: 'sub-1',
            subFolders: []
          }
        ]
      }
    };

    const mongoInstance: Storage = await StorageModel.create(storageInstance) as Storage;

    expect(mongoInstance.bla).to.be.equal(storageInstance.bla);
    expect(mongoInstance.root.displayName).to.be.equal(storageInstance.root.displayName);
    expect(mongoInstance.root.subFolders.length).to.be.equal(storageInstance.root.subFolders.length);
    expect(mongoInstance.root.subFolders[0].displayName).to.be.equal(storageInstance.root.subFolders[0].displayName);
  });
}

@hasezoey
Copy link
Member Author

hasezoey commented Jan 6, 2020

this is not a "valid" workaround, because an interface will get compiled to design:type of Object (reflection) and then in mongoose an Object will be converted to mongoose.Schema.Types.Mixed, which will allow any key:value pairs, without any validation

mongoInstance.path('root') instanceof mongoose.Schema.Types.Mixed // true

@dfseifert
Copy link

dfseifert commented Jan 6, 2020

Well yes you are right and thanks for pointing the design:type out, i didn't knew that.
But don't you think this might be a temporary solution for people who need Self-Containing or Nested-Self-Containing-Classes?

@hasezoey
Copy link
Member Author

But don't you think this might be a temporary solution for people who need Self-Containing or Nested-Self-Containing-Classes?

sorry, no this wouldnt be a temporary solution, because it wouldnt be "temporary" because JS will never support "infinite" self-nesting and would make the code less readable (and only have limited nesting like 1-5 levels or something like that)


Sorry that i didnt answer earlier, i just didnt really know what to say and needed to think about it

@mariusheine
Copy link

Is there any progress in solving this issue? It is a standard use case to model a parent child relationship between entities of equal type...

@hasezoey
Copy link
Member Author

@mariusheine if it is as sub-doc, then no, there is not an fix / solution, but if it is just references, it works, there you need to use the name of the referenced model

@mariusheine
Copy link

ahh yes. i was defining it as a ref but only inside the decorator. I did not set the type of property itself to Ref<...>.

thx a lot :D

hasezoey added a commit that referenced this issue May 16, 2020
- readme: remove babel from requirements
- quick-start-guide(gh-page): add "install" section
- known-issues(gh-page): restructure page with more detail
- known-issues(gh-page): add babel and workarounds
- shouldRun(test): add test "should allow self-referencing classes"

closes #269
mention #42
@hasezoey hasezoey changed the title Self-Containing-Classes & Nested-Self-Containing-Classes Self-Containing Classes May 17, 2020
@sebastiangug
Copy link

@hasezoey what is the use case of a self-containing class, is it more about providing info to the mongoose schema of what properties an object will have, when not using a ref?

@hasezoey
Copy link
Member Author

@sebastiangug sorry, i dont understand what you are trying to say, i dont know an use-case where an class should contain itself as an subdocument (i mean no ref)

@sebastiangug
Copy link

@hasezoey ah sorry, i just understood what it actually means, I was confusing it with something else -- my bad!

@andreialecu
Copy link
Contributor

Here's something related:
https://stackoverflow.com/questions/48050606/mongoose-schema-recursion

Mongoose has schema.add() for the purpose of recursive schemas: Automattic/mongoose#5289

Not sure if it can be handled automatically though.

@hasezoey
Copy link
Member Author

hasezoey commented Jul 3, 2020

@andreialecu sounds interesting, will try if i can find an good way to implement this (but i guess this will need an full schema analysis after the schemas are complete (maybe with an flag that marks that an property on the schema is recursive?))

@andreialecu
Copy link
Contributor

Indeed, some sort of schema analysis is probably needed to handle all cases. But I believe @lab9 was onto something by fixing the maximum call stack error.

Probably the starting point of implementing this is around here:

// ./src/prop.ts

export function _buildPropMetadata<T, U extends AnyParamConstructor<T>>(input: DecoratedPropertyMetadata, cl: U) {
  // ...
  
  if (utils.isNotDefined(Type)) {
    buildSchema(Type);
  }

  if (cl === Type) {
    // schema.add()?
    return;
  }

  // ...
}

@hasezoey
Copy link
Member Author

hasezoey commented Jul 3, 2020

@andreialecu the problem is in _buildPropMetadata there isnt an mongoose-schema yet, only schemas from data.ts so it will probably need to have an temporary class (probably called "Self") to filter for and apply in _buildSchema
-> one problem though, if an class is self-containing and extended in another, should the reference be the extended class or the "unextended"?

@MihaiWill
Copy link

MihaiWill commented Jul 15, 2020

Hi!

I have this field in Category class:

@Field((type) => [Category], { nullable: true })
  @Property({
    type: () => Category,
    default: [],
    required: false,
  })
  children?: Category[];

And I got this error:

RangeError: Maximum call stack size exceeded

Is there a way to add the class Category to it's field?

@hasezoey
Copy link
Member Author

@MihaiWill sorry, but your example dosnt include you class, so nothing can be said about the error yet (based from what is given, there shouldnt be an error)

@MihaiWill
Copy link

MihaiWill commented Jul 15, 2020

@hasezoey

@ObjectType()
export default class Category {
  @Field((type) => String)
  _id: string;

  @Field()
  @Property()
  title: string;

 @Field((type) => [Category], { nullable: true })
  @Property({
    type: () => Category,
    default: [],
    required: false,
  })
  children?: Category[];
}

@hasezoey
Copy link
Member Author

@MihaiWill you are trying to make an self-containing class as sub-documents, which isnt possible at the moment
-> but references should still work

@MihaiWill
Copy link

@hasezoey got it, solved with ref, thanks for answer!

@github-actions github-actions bot added the stale This Issue | PR had no activity in a while and will be closed if it stays so label Sep 1, 2020
@typegoose typegoose deleted a comment from github-actions bot Sep 1, 2020
@hasezoey hasezoey removed the stale This Issue | PR had no activity in a while and will be closed if it stays so label Sep 1, 2020
@rsaenen
Copy link

rsaenen commented Sep 29, 2020

I found another "solution" to keep type with a nested object:

@ObjectType()
export class NestedDto {

    @prop()
    @Field()
    public code: string;

    @prop({ type: NestedSubDto, default: [] })
    @Field(() => [NestedDto])
    public children: NestedDto[];

    @prop()
    public order: number;
}


export class NestedSubDto {

    @prop()
    public code: string;

    @prop({ type: NestedDto, default: [] })
    public children: NestedDto[];

    @prop()
    public order: number;
}

Works pretty well for now.

@hasezoey hasezoey added the cant fix This cant be fixed without external modules being updated label Aug 1, 2021
@hasezoey
Copy link
Member Author

closing this, because it will very likely never be able to get fixed

Note: self-references still work, but not self-nesting (subdocuments)

@hasezoey hasezoey closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cant fix This cant be fixed without external modules being updated has repro script This issue has an reproduce script help wanted Any help would be appreciated
Projects
None yet
Development

No branches or pull requests

7 participants