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

Fix decorator options mutating (fixes #55) #60

Merged
merged 3 commits into from
Oct 2, 2019
Merged

Fix decorator options mutating (fixes #55) #60

merged 3 commits into from
Oct 2, 2019

Conversation

ChrisLahaye
Copy link
Contributor

When a class has n properties using the same subclass, the
function baseProp is called n times for each property in subclass.

The function baseProp receives a parameter rawOptions which
should be copied as its being mutated later on. This is intended with
the statement:

rawOptions = Object.assign(rawOptions, {})

Although, this statement never changes the reference of rawOptions
with as result that the options are mutating. This causes the final schema
to be constructed without the options ref, itemsRef, refPath, etc when
the subclass is used more than once.

This commit fixes the mutation of the options.

Signed-off-by: Chris Lahaye dev@chrislahaye.com

When a class has n properties using the same subclass, the
function `baseProp` is called n times for each property in subclass.

The function `baseProp` receives a parameter `rawOptions` which
should be copied as its being mutated later on. This is intended with
the statement:
```
rawOptions = Object.assign(rawOptions, {})
```
Although, this statement never changes the reference of `rawOptions`
with as result that the options are mutating. This causes the final schema
to be constructed without the options ref, itemsRef, refPath, etc when
the subclass is used more than once.

This commit fixes the mutation of the options.

Signed-off-by: Chris Lahaye <dev@chrislahaye.com>
@hasezoey
Copy link
Member

hasezoey commented Oct 2, 2019

one question before i merge:
why define the new "rawOptions" after the first buildSchema? just to keep it constitent with the name assignment?

PS: could you update the branch to the latest master?

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only the one question i want answered before i merge, otherwise it looks good, thanks for pointing this reference thing out (I still dont understand what happened there)

@hasezoey
Copy link
Member

hasezoey commented Oct 2, 2019

@RyannGalea does this pr fix your issue?

@RyannGalea
Copy link

I have not tested this yet, I am hoping so!

@ChrisLahaye
Copy link
Contributor Author

I will merge master and move the assignment.

one question before i merge:
why define the new "rawOptions" after the first buildSchema? just to keep it constitent with the name assignment?

PS: could you update the branch to the latest master?

You could place it before the buildSchema between line 55 and 56, but the original statement that was intended to copy the object was also place afterwards.

Regarding the name consistency, you suggest naming the parameter back to rawOptions and then doing:

rawOptions = Object.assign({}, rawOptions);

The passed function in data_1.decoratorCache.get(initname).decorators.set(key, () => { may be executed multiple times, at least based on the following test:

    let i = false;
    data_1.decoratorCache.get(initname).decorators.set(key, () => {
        if (i) throw new Error('eh this is thrown');
        else i = true;

Assume parameter rawOptions has reference <1> which has value {ref: <99>, required: true}.
The first execution of rawOptions = Object.assign({}, rawOptions); causes rawOptions to have reference <2> which copied the value {ref: <99>, required: true} of <1>. Later we delete the key ref, causing <2> to have value {required: true}.
The second execution of rawOptions = Object.assign({}, rawOptions); causes rawOptions to have reference <3> which has copied the value {required: true} of <2>. This is wrong.

What we would like is the following.

Assume parameter rawOptions has reference <1> which has value {ref: <99>, required: true}.
The first execution of const rawOptions = Object.assign({}, origOptions); causes rawOptions to have reference <2> which copied the value {ref: <99>, required: true} of <1>. Later we delete the key ref, causing <2> to have value {required: true}.
The second execution of const rawOptions = Object.assign({}, origOptions); causes rawOptions to have reference <3> which has copied the value {ref: <99>, required: true} of <1>. This is good.

This is what happened before this commit.

Assume parameter rawOptions has reference <1> which has value {ref: <99>, required: true}.
The first execution of rawOptions = Object.assign(rawOptions, {}); causes rawOptions to still have reference <1> which has value {ref: <99>, required: true}. Later we delete the key ref, causing <1> to have value {required: true}.
The second execution of rawOptions = Object.assign(rawOptions, {}); causes rawOptions to still have reference <1> which has value {required: true}. This is wrong.

@hasezoey
Copy link
Member

hasezoey commented Oct 2, 2019

Regarding the name consistency, you suggest naming the parameter back to rawOptions and then doing:

with that i meant the below statement for name, didnt thought of the replacement of the statement before this pr, sorry

The second execution of rawOptions = Object.assign(rawOptions, {}); causes rawOptions to still have reference <1> which has value {required: true}. This is wrong.

i mean i dont understand why in this should have the previous pointer, i mean like it is a differing function call and Object.assign(rawOptions, {}) was meant so that "rawOptions" is always an object (and not an undefined or null or so)


anyway when this fixes these issues, i will merge it, even if i dont understand how these issues were caused

@hasezoey hasezoey merged commit 7d6e285 into typegoose:master Oct 2, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants