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.

Sign up for GitHub

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(repository): remove hidden properties from entities #1947

Closed
wants to merge 1 commit into from
Closed

fix(repository): remove hidden properties from entities #1947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/repository/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"@types/debug": "0.0.30",
"debug": "^4.0.1",
"lodash": "^4.17.10",
"loopback-datasource-juggler": "^4.0.0"
"loopback-datasource-juggler": "^4.1.2"
},
"files": [
"README.md",
Expand Down
2 changes: 1 addition & 1 deletion packages/repository/src/repositories/legacy-juggler-bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ export class DefaultCrudRepository<T extends Entity, ID>
}

protected toEntity(model: juggler.PersistedModel): T {
return new this.entityClass(model.toObject()) as T;
return new this.entityClass(model.toJSON()) as T;
}

protected toEntities(models: juggler.PersistedModel[]): T[] {
Expand Down
17 changes: 17 additions & 0 deletions packages/repository/test/unit/repositories/legacy-juggler-bridge.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,17 @@ describe('DefaultCrudRepository', () => {
properties: {
title: 'string',
content: 'string',
secret: 'string',
id: {name: 'id', type: 'number', id: true},
},
settings: {
hiddenProperties: ['secret'],
},
Copy link
Member

Choose a reason for hiding this comment

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

Let's stop growing the size of Note model please. Soon it will be way too large to reasonably work with.

Create a new model for the new hiddenProperties tests. This new model should be small and contain only properties needed to verify how hidden properties are treated.

});

title?: string;
content?: string;
secret?: string;
id: number;

constructor(data: Partial<Note>) {
Expand Down Expand Up @@ -137,6 +142,18 @@ describe('DefaultCrudRepository', () => {
expect(result.toJSON()).to.eql(note.toJSON());
});

it('hides hidden properties', async () => {
const repo = new DefaultCrudRepository(Note, ds);
const note = await repo.create({
title: 't3',
content: 'c3',
secret: 'secret',
});
expect(note.secret).to.be.undefined();
const result = await repo.findById(note.id);
expect(result.secret).to.be.undefined();
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong behavior. In LB4, the Repository API is the only way how to access and modify model data. How are we going to modify secret properties after your change?

IMO, the current implementation is correct, secret properties must stay available to JS/TS code.

What we may need to change is the way how CRUD Controller methods and/or REST layer converts the data returned by remote methods into JSON payload send to the clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hidden/protected(secret) properties are really interpreted as writeonly during serialization in LoopBack 3. They can be written but not read/search. Please note that a secret property can still be modified. For example:

repo.updateById(note.id, {secret: 'new-secret'});

But I agree with you that unconditionally removing them from CRUD is not ideal as we won't be able to support the use cases such as username/password verification.

I propose the following:

  1. Improve juggler so that options to control how to honor secret properties can be set at method level too in addition to datasource/model scope.

  2. Update CRUD controller code template/base class to hide secret properties for REST requests.

  3. For juggler.next, we need to reevaluate such property modifiers.

});

it('implements Repository.createAll()', async () => {
const repo = new DefaultCrudRepository(Note, ds);
const notes = await repo.createAll([
Expand Down