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

Data Model /Service Factory error generating manyToMany relationships #9075

Closed
yuning-zhang opened this issue Sep 10, 2024 · 4 comments
Closed

Comments

@yuning-zhang
Copy link

Bug report

Describe the bug

While generating manyToMany relationship between two models, retrieved relationship is not correct (using retrieve or list). This is possible due to ORM switching the select statement from different tables.

System information

Medusa version (including plugins): v2.0.8-preview
Node.js version: v20.12.2
Database: postgres (PostgreSQL) 14.12 (Homebrew)
Operating system: macOS 14.6
Browser (if relevant):

Steps to reproduce the behavior

Example repo of this bug https:/yuning-zhang/medusa-bug

  1. Set up this models
const Role = model.define("role", {
  id: model.id().primaryKey(),
  name: model.text().unique(),
  employees: model.manyToMany(() => Employee),
});

const Employee = model.define("employee", {
  id: model.id().primaryKey(),
  name: model.text().nullable(),
  first_name: model.text().nullable(),
  last_name: model.text().nullable(),
  email: model.text().unique(),
  roles: model.manyToMany(() => Role),
});
  1. Generate migrations and run
  2. Set up test route to generate example data
// src/api/company/test/route.ts

import { AuthenticatedMedusaRequest, MedusaResponse } from "@medusajs/medusa";
import B2BModuleService from "../../../modules/b2b/service";
import { B2B_MODULE } from "../../../modules/b2b";

type RequestBody = {};

export const POST = async (
  req: AuthenticatedMedusaRequest<RequestBody>,
  res: MedusaResponse
) => {
  const b2bModuleService: B2BModuleService = req.scope.resolve(B2B_MODULE);

  const role = await b2bModuleService.createRoles({
    name: "test_role",
  });

  const employee = await b2bModuleService.createEmployees({
    name: "Hello World",
    first_name: "Hello",
    last_name: "World",
    email: "[email protected]",
    roles: [role.id],
  });

  const fullEmployee = await b2bModuleService.retrieveEmployee(employee.id, {
    relations: ["roles"],
  });

  res.json({ role, employee, fullEmployee });
};

  1. Send request to test route
curl -X POST 'http://localhost:9000/company/test' \
-H 'Content-Type: application/json' \
--data-raw '{}'
  1. Received wrong response with roles not correct within employee. As you can see, role and employee are created correctly. But to retrieve the employee again with the roles relationship, the role id is actually the employee id, and the role name is the employee name
{
    -----------correct role created-------------
    "role": {
        "id": "01J7DA40JRN0FGDZABZZEPEA31",
        "name": "test_role",
        "employees": [],
        "created_at": "2024-09-10T06:32:16.216Z",
        "updated_at": "2024-09-10T06:32:16.216Z"
    },

    -----------correct employee created-------------
    "employee": {
        "id": "01J7DA40JZFENYDGKRTD056BZR",
        "name": "Hello World",
        "first_name": "Hello",
        "last_name": "World",
        "email": "[email protected]",
        "roles": [
            {
                "id": "01J7DA40JRN0FGDZABZZEPEA31"
            }
        ],
        "created_at": "2024-09-10T06:32:16.223Z",
        "updated_at": "2024-09-10T06:32:16.223Z"
    },

    -----------retrieve employee with id-------------
    "fullEmployee": {
        "id": "01J7DA40JZFENYDGKRTD056BZR",
        "name": "Hello World",
        "first_name": "Hello",
        "last_name": "World",
        "email": "[email protected]",
        "created_at": "2024-09-10T06:32:16.223Z",
        "updated_at": "2024-09-10T06:32:16.223Z",
        "deleted_at": null,
        "roles": [
            {
                -----------id here is same as employee id above?? and name?-------------
                "id": "01J7DA40JZFENYDGKRTD056BZR",
                "name": "Hello World",
                "created_at": "2024-09-10T06:32:16.223Z",
                "updated_at": "2024-09-10T06:32:16.223Z",
                "deleted_at": null
            }
        ]
    }
}

Expected behavior

The correct response in step 5 should be

{
    "fullEmployee": {
        "id": "01J7DA40JZFENYDGKRTD056BZR",
        "name": "Hello World",
        "first_name": "Hello",
        "last_name": "World",
        "email": "[email protected]",
        "created_at": "2024-09-10T06:32:16.223Z",
        "updated_at": "2024-09-10T06:32:16.223Z",
        "deleted_at": null,
        "roles": [
            {
                "id": "01J7DA40JRN0FGDZABZZEPEA31",
                "name": "test_role",
                "employees": [],
                "created_at": "2024-09-10T06:32:16.216Z",
                "updated_at": "2024-09-10T06:32:16.216Z"
            }
        ]
    }
}

Screenshots

If applicable, add screenshots to help explain your problem

Code snippets

Set up a public repo. Just need to run it locally with a database.

https:/yuning-zhang/medusa-bug

Additional context

Not all manyToMany relationships are broken, not sure why this particular set up causes ORM to error.

@adrien2p
Copy link
Member

Hey @yuning-zhang, just wanted to let you know that I am on it and ill get to the bottom of this issue

@adrien2p
Copy link
Member

@yuning-zhang could you update your role DML with the following please (including mappedBy)

const Role = model.define("role", {
  id: model.id().primaryKey(),
  name: model.text().unique(),
  employees: model.manyToMany(() => Employee, {
    mappedBy: "roles",
  }),
});

it should fix this issue, in the meantime I ll find a way to warn when this happens

@yuning-zhang
Copy link
Author

@adrien2p Thank you! That worked. I tested with adding mappedBy to Employee model too and it also fixed the issue. Do you recommend adding a mappedBy to one side of manyToMany relationship in general?

Feel free to close the ticket.

@adrien2p
Copy link
Member

    mappedBy: "roles",

Yes at least one side must have the mapped by, I ve created a PR to throw if it is not the case (#9085)

@shahednasser maybe we should add that as part of the doc?

kodiakhq bot pushed a commit that referenced this issue Sep 12, 2024
**What**
When both sides of a many to many relationship do not define the mapped by option, it leads to misconfigured relations and a malformed SQL query. (ref. #9075)

- When both mapped by are not defined, infer look up to try to identify the missing configuration if any
- Disallow defining many to many only on one side
@linear linear bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants