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

fix: long arrays in query converted to objects #17717

Closed
wants to merge 1 commit into from

Conversation

stafyniaksacha
Copy link

@stafyniaksacha stafyniaksacha commented Mar 6, 2023

Description

I issued same issue as explained here expressjs/body-parser#289

When using more that 20 fileds, directus don't get the query and return unrelevent results (ex: requested id when using items.readOne controller)

await directus
  .items('xxx')
  .readOne('xxx', {
    fields: [
      'field_1',
      // ...
      'field_21', // <- this one breaks
    ]
  })

Another solution that allowing Infinite in body-parser would be to update sanitizeFields to allow parsing objects

Type of Change

  • Bugfix
  • Improvement
  • New Feature
  • Refactor / codestyle updates
  • Other, please describe:

Requirements Checklist

  • New / updated tests are included
  • All tests are passing locally
  • Performed a self-review of the submitted code

If adding a new feature:

  • Documentation was added/updated. PR:

@paescuj
Copy link
Member

paescuj commented Mar 7, 2023

Thank you very much for this pull request!
However, for the reasons outlined in #16745 (comment) we cannot accept that as a solution, so I'll proceed to close this pull request. Thanks for the understanding!

@paescuj paescuj closed this Mar 7, 2023
@stafyniaksacha
Copy link
Author

@paescuj Sound good to me.

Do you think using csv value for fields instead of array in directus sdk by default is a viable option? (that would let directus sdk infer fields)

@paescuj
Copy link
Member

paescuj commented Mar 15, 2023

@paescuj Sound good to me.

Do you think using csv value for fields instead of array in directus sdk by default is a viable option? (that would let directus sdk infer fields)

How do you mean exactly?

@stafyniaksacha
Copy link
Author

Quering more than 20 fields won't work with directus sdk due to this limitation.
But sdk works when using coma separated value:

await directus
  .items('xxx')
  .readOne('xxx', {
    fields: [
      'field_1',
      // ...
      'field_21',
    ].join(',') as any // <- this convert fields to a string, so typescript can not infer the values, but it works
  })

Instead, we could do the join(',') inside the sdk before sending the request?

@stafyniaksacha stafyniaksacha deleted the patch-2 branch March 16, 2023 10:12
@paescuj
Copy link
Member

paescuj commented Mar 16, 2023

Quering more than 20 fields won't work with directus sdk due to this limitation. But sdk works when using coma separated value:

await directus
  .items('xxx')
  .readOne('xxx', {
    fields: [
      'field_1',
      // ...
      'field_21',
    ].join(',') as any // <- this convert fields to a string, so typescript can not infer the values, but it works
  })

Instead, we could do the join(',') inside the sdk before sending the request?

Ah I see, however I'd rather address the problem at its roots than add workaround patches to the SDK.

@stafyniaksacha
Copy link
Author

Ok, so dealing with keyed object inside directus where it attempt to loop to arrays?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants