bug: placeOrder error handling is incompatible with GraphQl norms

[damienwebdev]

This builds upon #39282

• As a developer, I want to use GraphQl in Magento and handle errors like other GraphQl applications.
• As a developer, I want placeOrder errors to trigger New Relic alarms.

Before the inclusion of errors on PlaceOrderOutput as a result of this commit if an error occured, the response to a failed place order would look like:

{
   errors: [
     { "message": " Something went wrong, etc etc..." } 
   ]
}

Most GraphQl clients interpret this as an error and appropriately cause error handling behaviors.

After the inclusion of 8e3a2e0#diff-ac993812eff9f7c3da3b41215860c173b0e83438a39cb16a1f3332672e2c8eab an error results in:

{
   data: {
     placeOrder: {
         order: null,
         errors: [
           { "message": " Something went wrong, etc etc..." } 
         ]
     }
   },
}

If anything, at the worst, this should be:

{
   data: {
     placeOrder: {
         order: null,
         errors: [
           { "message": " Something went wrong, etc etc..." } 
         ]
     }
   }
   errors: [
     { "message": " Something went wrong, etc etc..." } 
   ]
}

Error handling should match the common well-defined behaviors outlined by clients like Apollo. It should not be some bespoke poorly considered implementation.

Additionally, 8e3a2e0 inadvertently causes New Relic alarms to be silenced since failed placeOrder attempts are now returned as "success" as https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/GraphQl/Query/ErrorHandler.php#L53 is never called.

[m2-assistant]

Hi @damienwebdev. Thank you for your report.
To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce.

• For more details, review the Magento Contributor Assistant documentation.
• Add a comment to assign the issue: @magento I am working on this
• To learn more about issue processing workflow, refer to the Code Contributions.

---

Join Magento Community Engineering Slack and ask your questions in #github channel.
⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.
🕙 You can find the schedule on the Magento Community Calendar page.
📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

[JesKingDev]

+1 This is a huge issue and a breaking change for this mutation

In 2.4.7-p3, I get no errors at all. Calling placeOrder with anything wrong or missing (e.g. billing address, payment, etc.) returns the response

{
    "data": {
        "placeOrder": {
            "order": null
        }
    }
}

Unreal. This is a blocker for any upgrades or security patches. @damienwebdev thank you for reporting this issue.

[nwcasebolt]

Note that this same breaking pattern has already been introduced into the 2.4-develop branch for the UpdateCartItems resolver in this commit. If Adobe does not nip this in the bud now, it's going to spread.

[hnsr]

Just wanted to chime in and confirm the severity of this issue. Because exceptions are caught and transformed into items in the errors array, no exceptions are logged and placeOrder calls are failing silently.

The conversion of exceptions (into a pre-defined code + message) also seems to be done in such a way that it would not work properly if exception messages are translated (which is likely to be the case since it is catching LocalizedExceptions).

[m2-assistant]

Hi @engcom-Hotel. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

• 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
• 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
• 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
• 4. Verify that the issue is reproducible on 2.4-develop branch
  Details

  - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
• 5. Add label Issue: Confirmed once verification is complete.
• 6. Make sure that automatic system confirms that report has been added to the backlog.

[engcom-Hotel]

Hello @damienwebdev @JesKingDev @hnsr,

Thanks for the report and collaboration!

We have tried to reproduce the issue in the latest development branch i.e. 2.4-develop and we can create an order using the placeOrder graphQL.

I understand you are talking about an exception running the above query. Please let us know the reproduction steps for the same.

Thanks

[JesKingDev]

1. Create an empty guest cart

curl --location 'https://magento.test/graphql' \
--header 'Content-Type: application/json' \
--data '{"query":"mutation createEmptyCart ($input: createEmptyCartInput) {\n    createEmptyCart (input: $input)\n}","variables":{}}'

2. obtain guest cart ID from the response

3. Try to place order

curl --location 'https://magento.test/graphql' \
--header 'Content-Type: application/json' \
--data '{"query":"mutation placeOrder($input: PlaceOrderInput) {\n    placeOrder(input: $input) {\n        order {\n            order_number\n        }\n    }\n}","variables":{"input":{"cart_id":"GUESTCARTID"}}}'

Expected Response

{
  "errors": [
    {
      "message": "Guest email for cart is missing.",
      "locations": [
        {
          "line": 2,
          "column": 5
        }
      ],
      "path": [
        "placeOrder"
      ],
      "extensions": {
        "category": "graphql-input"
      }
    }
  ],
  "data": {
    "placeOrder": null
  }
}

Actual Response

{
  "data": {
    "placeOrder": {
      "order": null
    }
  }
}

The GraphQL spec per GraphQl.org requires errors to be separate from data. https://spec.graphql.org/October2021/#sec-Errors.Error-result-format

This is a breaking schema change that would require all of our GraphQL clients to change their behavior away from the GraphQl spec, such as popular clients like Apollo. https://www.apollographql.com/docs/react/data/error-handling#graphql-error-policies.

Further, this breaking schema change was initially introduced in a security patch which feels like an oversight and is a significant change that should at least warrant deprecation.

[engcom-Hotel]

Thanks @JesKingDev for the reply!

We ran the below mutation on Guest Cart for creating an order:

mutation {
  placeOrder(input: { cart_id: "GUESTCARTID" }) {
    errors {
      code
      message
    }
    order {
      order_id
      order_number
    }
    
  }
}

And we are able to get the proper exception as follows:

{
  "data": {
    "placeOrder": {
      "errors": [
        {
          "code": "CART_NOT_FOUND",
          "message": "Could not find a cart with ID \"GUESTCARTID\""
        }
      ],
      "order": null
    }
  }
}

Please refer to the screenshot below for reference:

Please let us know if we missed anything.

Thanks

[damienwebdev]

You missed something. Please read the original issue before you triage.

bug: placeOrder error handling is incompatible with GraphQl norms

Before the inclusion of errors on PlaceOrderOutput as a result of this commit if an error occured, the response to a failed place order would look like:

{
   errors: [
     { "message": " Something went wrong, etc etc..." } 
   ]
}

Most GraphQl clients interpret this as an error and appropriately cause error handling behaviors.

After the inclusion of 8e3a2e0#diff-ac993812eff9f7c3da3b41215860c173b0e83438a39cb16a1f3332672e2c8eab an error results in:

{
   data: {
     placeOrder: {
         order: null,
         errors: [
           { "message": " Something went wrong, etc etc..." } 
         ]
     }
   },
}

If anything, at the worst, this should be:

{
   data: {
     placeOrder: {
         order: null,
         errors: [
           { "message": " Something went wrong, etc etc..." } 
         ]
     }
   }
   errors: [
     { "message": " Something went wrong, etc etc..." } 
   ]
}

[hnsr]

Thanks @JesKingDev for the reply!

We ran the below mutation on Guest Cart for creating an order:

[...]

Please let us know if we missed anything.

Thanks

I believe you have acurately reproduced the problem with this; the errors are returned through a new placeOrder.errors field instead of the standard GraphQL error mechanism (on which all current Magento GraphQL integrations rely).

In addition to this:

• These errors (or any LocalizedException; so basically anything that can go wrong) are caught and not logged or rethrown making it impossile to track errors (not even through things like New Relic or Sentry), on arguably the most important mutation
• These errors are only reported with the correct code (such as CART_NOT_FOUND) if the locale of the store in which they happen is set to English

[engcom-Hotel]

Thanks @hnsr @damienwebdev for more information!

We got the point now. We are confirming this issue for further processing.

Thanks

[github-jira-sync-bot]

✅ Jira issue https://jira.corp.adobe.com/browse/AC-13357 is successfully created for this GitHub issue.

[m2-assistant]

✅ Confirmed by @engcom-Hotel. Thank you for verifying the issue.
Issue Available: @engcom-Hotel, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.