payload icon indicating copy to clipboard operation
payload copied to clipboard

fix: error 431 when having a lot of relationship items

Open andershermansen opened this issue 1 year ago • 2 comments

Description

To fix the issue of GET request getting a too long query variable as described in #6486, I have implemented X-HTTP-Method-Override header to allow this query to be sent as a POST request and giving the information that is it actually a GET request.

  • [X] I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • [X] Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [X] I have added tests that prove my fix is effective or that my feature works
  • [X] Existing test suite passes locally with my changes

andershermansen avatar May 24 '24 09:05 andershermansen

@andershermansen would you mind creating a test that tests for this?

It would need to be done under test/fields-relationship/e2e.spec.ts. I think it would be relatively simple.

Generate data with SDK

  1. Use the payload SDK client present in that file to create many items for a relatable collection, i.e. many relation-one docs
  2. Then use the SDK to create a doc for the fields-relationship collection and set the all the ID's (except for 1, save it for later!) created in step 1 to the relationshipHasMany field within it.

Use Playwright to test

  1. Then use Playwright to attempt to add that remaining relationship doc
  2. Ensure that the doc can be added successfully

The test should fail without your change and pass with it. If you can do this that would be very helpful! Let me know what you think.

JarrodMFlesch avatar May 24 '24 12:05 JarrodMFlesch

@JarrodMFlesch Thanks for the description on how to do the test. It helped a lot. This is my first time to write a playwright test, so there was some tweaking. Please review and let me know if something needs to be improved.

The test was very useful as it also got another case when I increased the amount of items even more (not_in is longer than in, so with even more items one more place failed).

Running the test on previous branch shows the failure, and on my branch it shows the success.

Here is the failure on beta branch, it times out waiting for the options which does not show up because of the API error: Skjermbilde 2024-05-24 kl  21 44 47

andershermansen avatar May 24 '24 19:05 andershermansen

@JarrodMFlesch A friendly reminder about this fix. If you want the fix or the test done in a different way, let me know and I can work on it.

andershermansen avatar Jun 03 '24 06:06 andershermansen

@DanRibbens Thanks. I have created and pushed docs as well.

andershermansen avatar Jun 11 '24 06:06 andershermansen

@andershermansen I am ready to merge this in! The only change I would request is the docs example. I think most of our users will feel more comfortable with a fetch example rather than an http example. Can you make that slight adjustment?

JarrodMFlesch avatar Jun 13 '24 14:06 JarrodMFlesch

I see you already adjusted it. Good change.

andershermansen avatar Jun 13 '24 19:06 andershermansen

@andershermansen I did, I want to merge it in and close your issue 👼

JarrodMFlesch avatar Jun 13 '24 19:06 JarrodMFlesch

Hi, quick question, is this also planned to be fixed in version 2. ?

NiklasSchmidts avatar Jul 15 '24 07:07 NiklasSchmidts