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(structure): provide better error handling if orderings contain invalid field #5709

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

jtpetty
Copy link
Contributor

@jtpetty jtpetty commented Feb 11, 2024

Description

Fixes SDX-847

Previously, if an invalid field name was given for an ordering for a schema, the user would be presented with a GROQ syntax error. This change adds a check in the event of an error to look for this situation to provide a more targeted error message.

Before:

image

After:

image

What to review

I am not sure if there is a better place to put this check. Since this is part of schema setup, this error should be rare, so I did not want to add this check in for every request. This is why I put it in the catch block. There may be other scenarios we want to check for here as well.

Testing

I did manual testing. I wasn't sure how to contrive this edge case in our test bench.

Notes for release

@jtpetty jtpetty requested a review from a team as a code owner February 11, 2024 17:52
@jtpetty jtpetty requested review from binoy14 and removed request for a team February 11, 2024 17:52
Copy link

vercel bot commented Feb 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Feb 26, 2024 1:07pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 26, 2024 1:07pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Feb 26, 2024 1:07pm

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Feb 11, 2024

Component Testing Report Updated Feb 26, 2024 1:12 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 34s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 13s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 12s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 33s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 0s 15 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 3s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ❌ Failed (Inspect) 24s 8 0 1

Copy link
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

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

I don't think this fully solves the problem, from my testing if you use the following ordering

 {
      title: 'Test',
      name: 'test',
      by: [
        {
          field: 'nonexistent',
          direction: 'desc',
        },
      ],
    },

than it will not error and make a query | order(nonexistent desc) From what I can tell this error only happens when you more than 2 fields to sort by and it fails due to running into , , in this line of code in groq you can query for fields non-existent and it will not error so that catch block might show this message for things that are not necessarily related to sort order of non existent fields

@jtpetty
Copy link
Contributor Author

jtpetty commented Feb 22, 2024

Thanks @binoy14! We do verify there is a missing field before displaying this particular message, but I will test it with the scenario you provided and see if I can get it to fail.

@jtpetty
Copy link
Contributor Author

jtpetty commented Feb 22, 2024

@binoy14 - in that line you reference, that function does the same logic (it's where I got it from) and it strips out unknown fields and prints a warning message to the console (not great). I tried to make it throw an error but it ended up crashing the whole page.

Copy link
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

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

Tested with different scenarios and not running into same issues as before. LGTM, thanks!

@jtpetty jtpetty added this pull request to the merge queue Mar 1, 2024
Merged via the queue into next with commit 4926b78 Mar 1, 2024
40 checks passed
@jtpetty jtpetty deleted the jtpetty/sdx-847 branch March 1, 2024 13:46
bjoerge pushed a commit that referenced this pull request Mar 1, 2024
…valid field (#5709)

* fix(structure): provide better error handling if orderings contain invalid field

* chore: fixed lint issue

* chore: fixed lint issue again

* fix(structure): provide better error handling if orderings contain invalid field

* fix(structure): provide better error handling if orderings contain invalid field

* fix(structure): add comment explaining why we are calling this method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants