Skip to content

Conversation

@InvictusMB
Copy link
Contributor

@InvictusMB InvictusMB commented Oct 4, 2020

Split from #5592

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@InvictusMB InvictusMB force-pushed the feature/ensure-filter-fields branch 2 times, most recently from 14cb5bd to 37254b7 Compare October 4, 2020 22:25
@bajtos bajtos added community-contribution feature Repository Issues related to @loopback/repository package labels Oct 5, 2020
Signed-off-by: InvictusMB <invictusmb@gmail.com>
@InvictusMB InvictusMB force-pushed the feature/ensure-filter-fields branch from 1346a2d to 09c29bd Compare October 7, 2020 15:44
@raymondfeng
Copy link
Contributor

@achrinza Would you like to review it?

@bajtos
Copy link
Member

bajtos commented Oct 13, 2020

@mdbetancourt could you please review the changes proposed in this pull request? I feel they may be related to the work you contributed in #6517.

Copy link
Contributor

@mdbetancourt mdbetancourt left a comment

Choose a reason for hiding this comment

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

@bajtos It looks good to me, @InvictusMB I think you can make some changes to make the code more understandable and avoid adding a library like lodash, I made the suggestions quickly so maybe you have to make some final adjustments

fieldsAdded: [] as (keyof T)[],
};
}
const isDisablingOnly = _.size(fields) > 0 && !_.some(fields, Boolean);
Copy link
Contributor

@mdbetancourt mdbetancourt Oct 14, 2020

Choose a reason for hiding this comment

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

this is some hard to understand a suggestion could be uses every instead of some negation a vanilla version:

Suggested change
const isDisablingOnly = _.size(fields) > 0 && !_.some(fields, Boolean);
const isDisablingOnly = Object.values(fields).length > 0 && Object.values(fields).every(f => !f);

Comment on lines +58 to +75
export function matchesFields<T extends object = AnyObject>(
fields: (keyof T)[],
filter?: Filter<T>,
) {
const normalized = new FilterBuilder(filter).build();
const targetFields = normalized.fields;
if (!targetFields) {
return true;
}
const isDisablingOnly =
_.size(targetFields) > 0 && !_.some(targetFields, Boolean);
for (const f of fields) {
if (!targetFields[f] && (f in targetFields || !isDisablingOnly)) {
return false;
}
}
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function matchesFields<T extends object = AnyObject>(
fields: (keyof T)[],
filter?: Filter<T>,
) {
const normalized = new FilterBuilder(filter).build();
const targetFields = normalized.fields;
if (!targetFields) {
return true;
}
const isDisablingOnly =
_.size(targetFields) > 0 && !_.some(targetFields, Boolean);
for (const f of fields) {
if (!targetFields[f] && (f in targetFields || !isDisablingOnly)) {
return false;
}
}
return true;
}
function isSuperset(set, subset) {
for (let elem of subset) {
if (!set.has(elem)) {
return false
}
}
return true
}
function difference(setA, setB) {
let _difference = new Set(setA)
for (let elem of setB) {
_difference.delete(elem)
}
return _difference
}

Comment on lines +24 to +30
const fields = builder.build().fields;
if (!fields || matchesFields(targetFields, filter)) {
return {
filter: builder.build(),
fieldsAdded: [] as (keyof T)[],
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fields = builder.build().fields;
if (!fields || matchesFields(targetFields, filter)) {
return {
filter: builder.build(),
fieldsAdded: [] as (keyof T)[],
};
}
const fields = new Set(Object.keys(builder.build().fields));
const target = new Set(Object.keys(targetFields))
if (!fields.size || isSuperset(target, fields)) {
return {
filter: builder.build(),
fieldsAdded: [],
};
}

Comment on lines +32 to +49
const fieldsAdded = (isDisablingOnly ? _.keys(fields) : []) as (keyof T)[];
targetFields.forEach(f => {
if (!fields[f]) {
fieldsAdded.push(f);
builder.fields(f);
}
});

const newFilter = builder.build();
// if the filter only hides the fields, unset the entire fields clause
if (isDisablingOnly) {
delete filter.fields;
}
return {
filter: newFilter,
fieldsAdded: _.uniq(fieldsAdded) as (keyof T)[],
} as const;
}
Copy link
Contributor

@mdbetancourt mdbetancourt Oct 14, 2020

Choose a reason for hiding this comment

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

Suggested change
const fieldsAdded = (isDisablingOnly ? _.keys(fields) : []) as (keyof T)[];
targetFields.forEach(f => {
if (!fields[f]) {
fieldsAdded.push(f);
builder.fields(f);
}
});
const newFilter = builder.build();
// if the filter only hides the fields, unset the entire fields clause
if (isDisablingOnly) {
delete filter.fields;
}
return {
filter: newFilter,
fieldsAdded: _.uniq(fieldsAdded) as (keyof T)[],
} as const;
}
const fieldsAdded = difference(target, fields)
fieldsAdded.forEach(builder.fields)
return {
filter: builder.build(),
fieldsAdded
} as const;
}

Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Almost LGTM 👍 I will review it again once those comments from @mdbetancourt are addressed.

@dhmlau
Copy link
Member

dhmlau commented Oct 15, 2020

@agnes512, assigning this PR to you since you're helping it to land. Thanks.

@agnes512
Copy link
Contributor

@InvictusMB any updates?

@stale
Copy link

stale bot commented Jul 14, 2021

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

@stale stale bot added the stale label Jul 14, 2021
@InvictusMB
Copy link
Contributor Author

Not stale. I'll have time to refresh this soon.

@stale stale bot removed the stale label Jul 27, 2021
@stale
Copy link

stale bot commented Sep 25, 2021

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

@stale stale bot added the stale label Sep 25, 2021
@stale
Copy link

stale bot commented Oct 9, 2021

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.

@stale stale bot closed this Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Repository Issues related to @loopback/repository package stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants