Allow refType to be defined in @prop and @arrayProp

[mfulop]

Merged changes from pr #149, solved conflicts, added itemsRefType (works the same as refType) to @arrayProps and tests

[coveralls]

Coverage increased (+0.9%) to 88.488% when pulling 189a6ce on mfulop:master into 0d68100 on szokodiakos:master.

[hasezoey]

when these are resolved, could you squash some of your commits?

i would recommend reverting 00ddb93 and then rebase, not merge :)

[mfulop]

Sure will do. vscode merge somehow messed up things, will try another way, sorry

[hasezoey]

i will review the changed in 60aa3ca tomorrow

[mfulop]

ok, you might want to look at ab91ee0 instead

[mfulop]

Btw though it’s working, I think Ref is now a bit too liberal. To be 100% correct, in Ref<R,T>, T should be R or the type of the designated id field of R.
Sadly it is beyond my current typescript knowledge :)

[hasezoey]

i would request setting the types to all lowercase to keep everything in style and not "string" and "Buffer" just to write it easy

PS: sorry that it took so long, my Desktop Environment just kept crashing

PPS: could you please squash all your commits? (git rebase -i master)
and try to reword your commits to comply with CONTRIBUTING.md (if not, i will at the merge squash & reword it)

[mfulop]

How about now?
Another thing came to mind before we finalize this. I merged Matt's changes from #149 regarding the refType, but does that have to be string? Would not specifying actual schema types make more sense? (it would to me...):
@prop({ ref: Car, refType: mongoose.Schema.Types.String })

[hasezoey]

I merged Matt's changes from #149 regarding the refType, but does that have to be string?

yes, would be a better idea, only that "I merged Matt's changes" doesnt not seem to be actually merged (its still string)

[mfulop]

OK, so here c0ab190 is how I would do it. I think it's much cleaner this way. What do you think?

[hasezoey]

I noticed after some testing, that if i do

const test = new RefTestModel({ refFieldString: new RefTestStringModel() }); // just as an example
if (isDocument(test.refFieldString)) {
  test.refFieldString // i mean here
}

the type of test.refFieldString in the if is InstanceType<T> | (string & Document) where it should only be InstanceType<T>

[hasezoey]

got it fixed, please modify typeguards.ts:

export function isDocument<T, S extends RefType>(doc: Ref<T, S>): doc is InstanceType<T> {

and

export function isDocumentArray<T, S extends RefType>(docs: Ref<T, S>[]): docs is InstanceType<T>[] {

and if possible, add a test for it (just to check if the types are not brocken)
otherwise it looks really great 👍

(example for the tests:

const test = new RefTestModel({ refArrayString: [new RefTestStringModel()] });
if (isDocumentArray(test.refArrayString)) {
  test.refArrayString;
  console.log('in if isDocument');
} else {
  console.log('in else isDocument');
}

const test2 = new RefTestModel({ refFieldString: new RefTestStringModel() });
if (isDocument(test2.refFieldString)) {
  test2.refFieldString;
  console.log('in if isDocument');
} else {
  console.log('in else isDocument');
}

but please note that this only the thing i used to get it working, please modify it so that it uses expect and throw)

[mfulop]

I implemented all the changes and added the test. That actually brought up this:
Shouldn't
export type Ref<R, T extends RefType = mongoose.Schema.Types.ObjectId> = R | T;
be
export type Ref<R, T extends RefType = mongoose.Types.ObjectId> = R | T;
instead? (the actual values are not SchemaTypes but ObjectIds).

But would this change break compatibility?

[hasezoey]

a mongoose issue explaining the difference (for further readers) Automattic/mongoose#1671

so yes, it would be more right to use Types.T insteace of Schema.Types.T
and i cant see any obvious "breaking" changes, "Ref<T, S>" is just a type and so-or-so it works as expected

so i would say, yes it would be good to keep it so

[mfulop]

This: new RefTestModel({ refField: mongoose.Types.ObjectId() }) is actually not possible without casting when using SchemaType

[hasezoey]

This: new RefTestModel({ refField: mongoose.Types.ObjectId() }) is actually not possible without casting when using SchemaType

ok, then lets keep it at Schema.Types.T

[mfulop]

Then its done ;-)

[hasezoey]

Everything is and looks good, just some small style changes

[mfulop]

Could you check something for me?

I am putting together the test you requested above. When running the below snippet, the created RefTestModel has both refFieldString and refFieldNumber fields created with the corresponding ID.
However, while the RefTestNumber document is created in the database, for some reason, the RefTestString document is not. Therefore, the next populate will not find it.
What makes the difference? Or is it a race condition somewhere?

await RefTestModel.create({
      refFieldString: new RefTestStringModel({ _id: 'test1' }), // document not created
      refFieldNumber: new RefTestNumberModel({ _id: 1234 }), // document created
    });

[mfulop]

By using create refFieldString: await RefTestStringModel.create({ _id: 'test1' }) it works (of course) but I guess that's not the point.

Similarly strange:

refArrayNumber: [new RefTestNumberModel({ _id: 5678 }), new RefTestNumberModel({ _id: 9876 })]

puts both IDs to the field correctly but only creates the first document in the database so populate returns an array with 1 item.

Should it even work?

[hasezoey]

Or is it a race condition somewhere?

typegoose nowhere sets a race condition, at least not knowingly

---

could you push it to another branch so i can check it out?

[mfulop]

https://github.com/mfulop/typegoose/tree/reftests/

[hasezoey]

However, while the RefTestNumber document is created in the database, for some reason, the RefTestString document is not.

sorry, cant confirm that, for me everything gets saved
modified it to

const { _id: populatedId } = await RefTestModel.create({
  refField: new RefTestModel(), // gets saved
  refArray: [new RefTestModel(), new RefTestModel()], // gets saved
  refFieldString: new RefTestStringModel({ _id: 'test1' }), // gets saved
  refArrayString: [
    new RefTestStringModel({ _id: 'test2' }), // gets saved
    new RefTestStringModel({ _id: 'test3' }) // gets saved
  ],
  refFieldNumber: new RefTestNumberModel({ _id: 1234 }), // gets saved
  refArrayNumber: [new RefTestNumberModel({ _id: 5678 }), new RefTestNumberModel({ _id: 9876 })], // gets saved
  refFieldBuffer: new RefTestBufferModel({ _id: Buffer.from([1, 2, 3, 4]) }), // gets saved
  refArrayBuffer: [
    new RefTestBufferModel({ _id: Buffer.from([5, 6, 7, 8]) }), // gets saved
    new RefTestBufferModel({ _id: Buffer.from([9, 8, 7, 6]) }) // gets saved
  ]
});

db document:

{
  "_id": "5d4733adb736993b51db6a45",
  "refArray": [
    "5d4733adb736993b51db6a43",
    "5d4733adb736993b51db6a44"
  ],
  "refArray2": [],
  "refArrayString": [
    "test2",
    "test3"
  ],
  "refArrayString2": [],
  "refArrayNumber": [
    5678,
    9876
  ],
  "refArrayNumber2": [],
  "refArrayBuffer": [
    "BQYHCA==",
    "CQgHBg=="
  ],
  "refArrayBuffer2": [],
  "refField": "5d4733adb736993b51db6a42",
  "refFieldString": "test1",
  "refFieldNumber": 1234,
  "refFieldBuffer": "AQIDBA==",
  "__v": 0
}

(Note: mongodb compass exported it as json, so example buffer got converted to string, but i assure you, everything is saved how it should)

[mfulop]

Do tests run without an error on the reftests branch for you?

[mfulop]

Oh yes, the root document is correctly created. But do the referenced documents (eg. the one with id 5678) all exist?

[hasezoey]

Do tests run without an error on the reftests branch for you?

no thests do not run, because not populated properly

Oh yes, the root document is correctly created. But do the referenced documents (eg. the one with id 5678) all exist?

i dont have any with the id with 5678, only an array of numbers

[hasezoey]

currently it fails at expect(foundPopulated.refArray.length).to.equal(2);, with AssertionError: expected 0 to equal 2

[mfulop]

currently it fails at expect(foundPopulated.refArray.length).to.equal(2);, with AssertionError: expected 0 to equal 2

yes and that means populate can't find the foreign document with that id

[hasezoey]

yes and that means populate can't find the foreign document with that id

i can confirm that Field refArrayNumber documents are saved on the DB

[mfulop]

Ok, I will dig deeper tomorrow to see what’s going on. If the documents are there and the ids are there then populate should work.

[hasezoey]

got it further with

const pop = Object.keys(RefTestModel.schema.obj);
const foundPopulated = await RefTestModel.findById(populatedId).populate(pop).exec();

its still the same error, only for refArrayString this time, but many others are populated

---

will investigate further

Update:
all properties from refField to refArrayString do not get populated, but all the others

Update2:
i searched through the schema, but didnt find any differences (except types) between refFieldString and refFieldNumber

Update3:
after some much more testing i found that refField has an id (in the DB), but the actual document dosnt exist... (same with refFieldString)

[mfulop]

First thing I will try in the morning is the same with native mongoose (and a real mongodb instance)

[hasezoey]

Update(4):
i noticed after disabling all other tests, that the documents were never created (including the ones i said are existing), because they have duplicate ids (like the number 5678 is used 3 times)

TL;DR: what i said was wrong, no new refs are actually saved

Edit:
i read more about it, it is a mongoose "by design" because it is a reference, not a subdocument
Automattic/mongoose#2228
-> we could implement such things, but this would mean that we would need to redo the .create / .save functions OR do an "invisible" pre-save-hook
--> but it is out of scope for this PR

[mfulop]

OK, so it is not supposed to work. Then await create() will do for tests, give me a minute

[mfulop]

-> we could implement such things, but this would mean that we would need to redo the .create / .save functions OR do an "invisible" pre-save-hook
My 2 cents - I would not... it's a quite big difference compared to how mongoose behaves on its own, could be misleading for the ones with Mongoose experience

--> but it is out of scope for this PR
I agree

[hasezoey]

i see no problem anymore, only resolve the last review, squash your commits, and i will approve 👍

[mfulop]

There you go 189a6ce

And thanks for all :)

[hasezoey]

Everything got resolved, everything conforms with the Coding Style, Commits are structured right 👍

[hasezoey]

@Ben305 (if you are back) i would want to know if this should be in release 6.0.0 or release 5.10.0?

[mfulop]

Hi!

Sorry, I was out for a bit. Actually I would appreciate this before v6. I am planning to port some of our proprietary sw to Typegoose and this was one missing feature. (There might be more but then you’ll see a PR about them anyways :) ). Unfortunately this project is now iced for a few days but I’ll be back.

[hasezoey]

Unfortunately this project is now iced for a few days but I’ll be back.

which project? this pr is finished an ready to go :) (ben is out for a week)

[mfulop]

The one where I am porting stuff to run on Typegoose :)

[hasezoey]

Merged it for now into 6.0.0(-19) 27f5d59

Edit / Update:
I (still) dont think that this change will be in a 5.x version anymore

[hasezoey]

this can be closed because it is included in v6.0.0, and will not be included a v5.x release

@Ben305