Skip to content

fix(polymorphism): support orderBy with base fields #1086

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

Merged
merged 2 commits into from
Mar 6, 2024
Merged

fix(polymorphism): support orderBy with base fields #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d345b80
fix(polymorphism): support `orderBy` with base fields
ymc9 Mar 6, 2024
f85a6a1
fix test
ymc9 Mar 6, 2024
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
34 changes: 22 additions & 12 deletions packages/runtime/src/enhancements/delegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ export class DelegateProxyHandler extends DefaultPrismaProxyHandler {
this.injectWhereHierarchy(model, args?.where);
this.injectSelectIncludeHierarchy(model, args);

if (args.orderBy) {
// `orderBy` may contain fields from base types
args.orderBy = this.buildWhereHierarchy(this.model, args.orderBy);
}

if (this.options.logPrismaQuery) {
this.logger.info(`[delegate] \`${method}\` ${this.getModelName(model)}: ${formatObject(args)}`);
}
Expand Down Expand Up @@ -126,19 +131,19 @@ export class DelegateProxyHandler extends DefaultPrismaProxyHandler {
});
}

private buildWhereHierarchy(where: any) {
private buildWhereHierarchy(model: string, where: any) {
if (!where) {
return undefined;
}

where = deepcopy(where);
Object.entries(where).forEach(([field, value]) => {
const fieldInfo = resolveField(this.options.modelMeta, this.model, field);
const fieldInfo = resolveField(this.options.modelMeta, model, field);
if (!fieldInfo?.inheritedFrom) {
return;
}

let base = this.getBaseModel(this.model);
let base = this.getBaseModel(model);
Comment on lines +134 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of buildWhereHierarchy method correctly handles the transformation of where conditions to support fields inherited from base types. This is key to achieving the PR's objective of enhancing polymorphism support. However, there's a potential risk of prototype pollution due to direct manipulation of object properties without validating their names against a list of disallowed prototype properties. This could lead to unintended side effects if a user-controlled input is passed directly to this method.

Consider adding a check to ensure that field names do not correspond to prototype properties, or use a Map for handling dynamic keys safely.

let target = where;

while (base) {
Expand Down Expand Up @@ -173,12 +178,17 @@ export class DelegateProxyHandler extends DefaultPrismaProxyHandler {

for (const kind of ['select', 'include'] as const) {
if (args[kind] && typeof args[kind] === 'object') {
for (const [field, value] of Object.entries(args[kind])) {
if (value !== undefined) {
for (const [field, value] of Object.entries<any>(args[kind])) {
const fieldInfo = resolveField(this.options.modelMeta, model, field);
if (fieldInfo && value !== undefined) {
if (value?.orderBy) {
// `orderBy` may contain fields from base types
value.orderBy = this.buildWhereHierarchy(fieldInfo.type, value.orderBy);
}

if (this.injectBaseFieldSelect(model, field, value, args, kind)) {
delete args[kind][field];
} else {
const fieldInfo = resolveField(this.options.modelMeta, model, field);
if (fieldInfo && this.isDelegateOrDescendantOfDelegate(fieldInfo.type)) {
let nextValue = value;
if (nextValue === true) {
Expand Down Expand Up @@ -847,15 +857,15 @@ export class DelegateProxyHandler extends DefaultPrismaProxyHandler {
args = deepcopy(args);

if (args.cursor) {
args.cursor = this.buildWhereHierarchy(args.cursor);
args.cursor = this.buildWhereHierarchy(this.model, args.cursor);
}

if (args.orderBy) {
args.orderBy = this.buildWhereHierarchy(args.orderBy);
args.orderBy = this.buildWhereHierarchy(this.model, args.orderBy);
}

if (args.where) {
args.where = this.buildWhereHierarchy(args.where);
args.where = this.buildWhereHierarchy(this.model, args.where);
}

if (this.options.logPrismaQuery) {
Expand All @@ -875,11 +885,11 @@ export class DelegateProxyHandler extends DefaultPrismaProxyHandler {
args = deepcopy(args);

if (args?.cursor) {
args.cursor = this.buildWhereHierarchy(args.cursor);
args.cursor = this.buildWhereHierarchy(this.model, args.cursor);
}

if (args?.where) {
args.where = this.buildWhereHierarchy(args.where);
args.where = this.buildWhereHierarchy(this.model, args.where);
}

if (this.options.logPrismaQuery) {
Expand Down Expand Up @@ -915,7 +925,7 @@ export class DelegateProxyHandler extends DefaultPrismaProxyHandler {
args = deepcopy(args);

if (args.where) {
args.where = this.buildWhereHierarchy(args.where);
args.where = this.buildWhereHierarchy(this.model, args.where);
}

if (this.options.logPrismaQuery) {
Expand Down
1 change: 1 addition & 0 deletions tests/integration/tests/cli/plugins.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ describe('CLI Plugins Tests', () => {
strict: true,
lib: ['esnext', 'dom'],
esModuleInterop: true,
skipLibCheck: true,
},
})
);
Expand Down
35 changes: 35 additions & 0 deletions tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,41 @@ describe('Polymorphism Test', () => {
expect(imgAsset.owner).toMatchObject(user);
});

it('order by base fields', async () => {
const { db, user } = await setup();

await expect(
db.video.findMany({
orderBy: { viewCount: 'desc' },
})
).resolves.toHaveLength(1);

await expect(
db.ratedVideo.findMany({
orderBy: { duration: 'asc' },
})
).resolves.toHaveLength(1);

await expect(
db.user.findMany({
orderBy: { assets: { _count: 'desc' } },
})
).resolves.toHaveLength(1);

await expect(
db.user.findUnique({
where: { id: user.id },
include: {
ratedVideos: {
orderBy: {
viewCount: 'desc',
},
},
},
})
).toResolveTruthy();
});
Comment on lines +238 to +271
Copy link
Contributor

Choose a reason for hiding this comment

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

The new test case order by base fields correctly tests the functionality of ordering by base fields in different scenarios, which aligns with the PR's objectives to enhance polymorphism support. However, there are a few points to consider for improvement:

  1. Assertion Specificity: The test case uses .resolves.toHaveLength(1) to assert the outcome of the findMany and findUnique operations. While this checks that the query returns a result, it does not verify that the ordering is correctly applied. It would be beneficial to enhance the assertions to check the order of the returned records explicitly, ensuring that the orderBy functionality works as expected.

  2. Error Handling: The test case uses .toResolveTruthy() for the findUnique operation, which is not a standard Jest matcher. This might be a typo or a custom matcher not defined in the provided context. If it's intended to check that the promise resolves successfully, consider using .resolves.toBeTruthy() or another appropriate Jest matcher for clarity and correctness.

  3. Test Coverage: The test case covers ordering by viewCount and duration for video and ratedVideo models, and a count-based order for assets related to a user. To further ensure robustness, consider adding more scenarios that cover edge cases, such as ordering by fields that might have null values, or combining orderBy with other query parameters like where and include to test more complex queries.

  4. Documentation: Adding comments to explain the setup and the specific scenarios being tested can improve the readability and maintainability of the test case. This is especially useful for future developers who might work on this test suite.

Overall, the test case is a good start towards validating the new functionality but could be enhanced for more comprehensive testing and clarity.

  • Consider enhancing the assertions to explicitly check the order of the returned records.
  • Verify the usage of .toResolveTruthy() and replace it with a standard Jest matcher if necessary.
  • Expand the test coverage to include more scenarios and edge cases.
  • Add comments to document the setup and scenarios being tested for better readability and maintainability.


it('update simple', async () => {
const { db, videoWithOwner: video } = await setup();

Expand Down