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

IgnoreProperty Decorator problem when inheriting from parent model #503

Closed
1 task
engpire1991 opened this issue Jan 3, 2019 · 5 comments · May be fixed by Mfuon2/tsed#10
Closed
1 task

IgnoreProperty Decorator problem when inheriting from parent model #503

engpire1991 opened this issue Jan 3, 2019 · 5 comments · May be fixed by Mfuon2/tsed#10
Assignees
Milestone

Comments

@engpire1991
Copy link

engpire1991 commented Jan 3, 2019

Information

  • Version: 4.x/5.x
  • Type: Bug

Description

IgnoreProperty decorator is not working when model class inherits a base class with Property decorator set.
Other decorators, for example Required, work correctly on the new model.
Note: This is the first time i am making an issue, so please let me know if something is missing.

Example

// Models

class Product {
    @Property()
    name: string;

    @Property()
    categoryId: number;

    @Property()
    description: string;
}

@ModelStrict(true)
export class UserProductPostModel extends Product{
    @Required()
    name: string;

    @IgnoreProperty()
    categoryId: number;

    @Property()
    description: string;
}

@ModelStrict(true)
export class AdminProductPostModel extends Product{
    @Required()
    name: string;

    @Required()
    categoryId: number;

    @Property()
    description: string;
}

// Controller

@Post("/admin")
public async postProduct(
	@BodyParams() strictBodyCheck: AdminProductPostModel,
): Promise<AdminProductPostModel> {
	console.log(strictBodyCheck.categoryId);
	// correctly throws if categoryId is not set
	// swagger documentation correctly shows categoryId as required
	return strictBodyCheck;
}

@Post("/")
public async postProduct(
	@BodyParams() strictBodyCheck: UserProductPostModel ,
): Promise<UserProductPostModel>{
	console.log(strictBodyCheck.categoryId);
	// wont throw if received even though ModelStrict is set to true
	// swagger documentation also shows categoryId as property although it should be ignored.
	// categoryId is also sent in response
	return strictBodyCheck;
}

Acceptance criteria

  • properties with IgnoreProperty decorator are properly handled on child classes
@engpire1991
Copy link
Author

// Update
The culprit seems to be the getProperties function of PropertyRegistry:

export class PropertyRegistry {
 ....
	static getProperties(target: Type<any>): Map<string | symbol, PropertyMetadata> {
		const map = new Map<string | symbol, PropertyMetadata>();

		ancestorsOf(target).forEach(klass => {
			this.getOwnProperties(klass).forEach((v: PropertyMetadata, k: string | symbol) => {
				if (!v.ignoreProperty) {
					map.set(k, v);
				}
			});
		});

		return map;
	}
}

Not sure if it is intended, but if ancestor class has the property without ignoreProperty flag set, then it gets added to the map, but is not removed if child class has the same property with ignoreProperty flag set to true.

I locally made a test and two options worked
Option 1) remove the property from map whenever ignoreProperty is true

if (!v.ignoreProperty) {
	map.set(k, v);
} else {
	map.delete(k);
}

Problem with this approach is that now, if parent class will have IgnoreProperty set to true, but the child class won't, then the property will still get added to the map

Option 2) Keep a list of ignored properties and skip them

static getProperties(target: Type<any>): Map<string | symbol, PropertyMetadata> {
	const map = new Map<string | symbol, PropertyMetadata>();
	const ignored: string[] = [];
	
	ancestorsOf(target).forEach(klass => {
		this.getOwnProperties(klass).forEach((v: PropertyMetadata, k: string | symbol) => {
			if (ignored.indexOf(k) !== -1){
				return;
			}
			if (!v.ignoreProperty) {
				map.set(k, v);
			} else {
				map.delete(k);
				ignored.push(k);
			}
		});
	});

	return map;
}

@Romakita
Copy link
Collaborator

Romakita commented Jan 3, 2019

Hello @engpire1991
Can you try this solution:

PropertyMetadata:

  /**
   *
   * @returns {boolean}
   */
  get ignoreProperty(): boolean {
    return this.store.get("ignorePropety");
  }

  /**
   *
   * @param {boolean} value
   */
  set ignoreProperty(value: boolean) {
    this.store.set("ignorePropety", value)
  }

Tell if it works :)
See you,
romain

@engpire1991
Copy link
Author

Hello @Romakita

I tried updating PropertyMetadata, but still got the same results.

i added some debug logs and here is what i get

@ModelStrict(true)
export class UserProductPostModel extends Product{
    @Required()
    name: string;

    @IgnoreProperty()
    @Test()
    categoryId: number;

    @Property()
    description: string;
}

function Test(): Function {
    return (target: any, propertyKey: string) => {
        setTimeout(() => {
            console.log(Store.from(target, "categoryId"));
            console.log(PropertyRegistry.getProperties(target));
        }, 1000);

    }
}

// In PropertyRegistry

        if (!v.ignoreProperty) {
          map.set(k, v);
          console.log(`setting property on ${v.targetName} property ${v.name} for ${getClass(target)}`);
        } else {
          console.log(`ignoring property on ${v.targetName} property ${v.name} for ${getClass(target)}`);
        }

after the timeout it logs out:

Store {
	_map:
	Map {
		'schema' => JsonSchema {
			_proxy: [JsonSchema],
			_refName: 'Number',
			_type: 'number'
		},
		'ignoreProperty' => true
	}
}

setting property on Product property name for class UserProductPostModel extends Product {
}
setting property on Product property categoryId for class UserProductPostModel extends Product {
}
setting property on Product property description for class UserProductPostModel extends Product {
}
setting property on UserProductPostModel property name for class UserProductPostModel extends Product {
}
ignoring property on UserProductPostModel property categoryId for class UserProductPostModel extends Product {
}
setting property on UserProductPostModel property description for class UserProductPostModel extends Product {
}

Map {
	'name' => PropertyMetadata {
		_target: UserProductPostModel {},
		_propertyKey: 'name',
		_type: [Function: String],
		_name: 'name',
		_store: Store {
			_map: [Map]
		},
		_allowedRequiredValues: [],
		_ignoreProperty: false
	},
	'categoryId' => PropertyMetadata {
		_target: Product {},
		_propertyKey: 'categoryId',
		_type: [Function: Number],
		_name: 'categoryId',
		_store: Store {
			_map: [Map]
		},
		_allowedRequiredValues: [],
		_ignoreProperty: false
	},
	'description' => PropertyMetadata {
		_target: UserProductPostModel {},
		_propertyKey: 'description',
		_type: [Function: String],
		_name: 'description',
		_store: Store {
			_map: [Map]
		},
		_allowedRequiredValues: [],
		_ignoreProperty: false
	}
}

So the 'ignoreProperty' is correctly set in the Store. The property gets correctly ignored on UserProductPostModel model, but is instead taken from the base Product model.

@Romakita Romakita added this to the BACKLOG milestone Jan 24, 2019
@Romakita Romakita self-assigned this Jan 24, 2019
@Romakita
Copy link
Collaborator

Fixed with v5.1.1 :)

@engpire1991
Copy link
Author

Great, thank you :)

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

Successfully merging a pull request may close this issue.

2 participants