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

Add omitempty to JSON field tags on optional fields #1937

Open
m-mattia-m opened this issue Oct 17, 2024 · 11 comments
Open

Add omitempty to JSON field tags on optional fields #1937

m-mattia-m opened this issue Oct 17, 2024 · 11 comments
Labels

Comments

@m-mattia-m
Copy link

m-mattia-m commented Oct 17, 2024

Describe the bug

When my API receives a webhook request from Stripe via the CLI, I got the error expected required property account to be present and the location attribute is: body. After I created a manual request with the same body and added the attribute account (which is an empty string) I got the same error for the attribute: previous_attributes at the body.data location. After adding this element as well, it worked.

I checked the stripe docs and saw that the account attribute is a nullable string. The previous_attributes is a nullable map. However, the stripe-go-sdk regard these attributes as required attributes and not as optional.

Consider this pull request: #1936

To Reproduce

  1. Create a simple Go API which tries binding the request body to stripe.Event. Consider that if you
  2. Forward Stripe webhook events to your local device (stripe listen --forward-to localhost:8081/webhook)
  3. Create an event (I created a payment via the UI, which had a valid customer)

Check out this repository (m-mattia-m/stripe-go-event-bug-report) for a working example and all the request and response details (cURL, JSON, ...).

Expected behavior

I expect that I can use the stripe.Event object to bind the JSON and it works fine. It should also be able to handle optional attributes like account or previous_attributes.

Code snippets

package main

import (
	"context"
	"fmt"
	"github.com/danielgtaylor/huma/v2"
	"github.com/danielgtaylor/huma/v2/adapters/humagin"
	"github.com/gin-gonic/gin"
	"github.com/stripe/stripe-go/v80"
	"log"
	"net/http"
)

type StripeWebhookEventRequest struct {
	Body stripe.Event `json:"body" bson:"body"`
}

func main() {

	router := gin.New()
	humaConfig := huma.DefaultConfig("Stripe Webhook example", "1.0.0")
	humaConfig.Servers = []*huma.Server{
		{URL: "http://localhost:8080"},
	}
	api := humagin.New(router, humaConfig)

	huma.Register(api, huma.Operation{
		Method:      http.MethodPost,
		OperationID: "receive-stripe-webhook",
		Summary:     "Receive stripe webhook",
		Description: "Receives stripe webhook events.",
		Path:        "/webhook",
	}, GetColumnSchema())

	err := router.Run(":8080")
	if err != nil {
		log.Fatal(err.Error())
	}

}

func GetColumnSchema() func(c context.Context, input *StripeWebhookEventRequest) (*StripeWebhookEventRequest, error) {
	return func(c context.Context, input *StripeWebhookEventRequest) (*StripeWebhookEventRequest, error) {
		fmt.Println("---------- [ Successfully received Stripe event ] ----------")
		fmt.Println(input.Body)

		return nil, nil
	}
}

OS

macOS 15.0.1

Go version

Go 1.21

stripe-go version

v80.2.0

API version

2022-11-15

Additional context

No response

@m-mattia-m m-mattia-m added the bug label Oct 17, 2024
@jar-stripe
Copy link
Contributor

jar-stripe commented Oct 18, 2024

@m-mattia-m Thank you! I've added this to our internal backlog so we can track and prioritize it alongside our other work.

@mbroshi-stripe
Copy link
Contributor

mbroshi-stripe commented Jan 29, 2025

👋 Hi @m-mattia-m! Thanks for raising this issue! I'm trying to understand what you mean by

However, the stripe-go-sdk regard these attributes as required attributes and not as optional.

In what sense are they required? I looked at your sample PR #1936 (thanks for putting that together!), but that looks to be addressing the json.Marshal behavior.

Also, have you looked at how we handle webhooks using webhook.ConstructEvent in our sample projects like: https://github.com/stripe-samples/checkout-one-time-payments/blob/main/server/go/server.go#L131-L155? That might be a pattern you can follow.

@m-mattia-m
Copy link
Author

Hi @mbroshi-stripe 👋 Thanks for facing this issue.

If I remember correctly, was my application with the official stripe-go struct not able to bind the request that is made on the object due to missing attributes. So what I meant was that the stripe-go struct expected to get a complete object, but in reality there were three missing attributes.

What I tried in the mentioned PR was, that I make the attributes optional because if it's not decelerated as omitempty it will be handled as required.

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string. Source: pkg.go.dev

So I think the json.Marshal behavior is correct because it expected a value, e.g., account because there isn't a property that says that this attribute shouldn't be there, and therefore it expects to have this attribute. If you define them as omitempty, json.Marshal would check if the attribute is in the JSON it received, but if there isn't an attribute, it will go further because we say that it could be possibly null (or left out).

I hope it's understandable what I mean. 😬

@mbroshi-stripe
Copy link
Contributor

I apologize, but I'm still having trouble understanding this bit:

So what I meant was that the stripe-go struct expected to get a complete object

What does that mean the "struct expected to get a complete object"? Perhaps a code snippet might help me understand what you're trying to accomplish. If I take the provided example in this Issue and just replace StripeWebhookEventRequest with stripe.Event in GetColumnSchema, it runs without issue.

@m-mattia-m
Copy link
Author

m-mattia-m commented Jan 31, 2025

Now I'm not sure anymore if I have a thinking error, but I still have this error. :)

I opened my playground repository m-mattia-m/stripe-go-event-bug-report and started the application on port 8080. Then I started my terminal and logged into Stripe with stripe login after this, I started the webhook forwarding with stripe listen --forward-to localhost:8080/webhook --log-level debug.

Now all applications are up and running, and I triggered the first event with stripe trigger payment_intent.succeeded. In the tab where I forwarded the traffic, I can see this line:

[...] Incoming message message={"type":"webhook_event"," [...]
2025-01-31 19:56:01 --> payment_intent.created [evt_asdf]
2025-01-31 19:56:01 <-- [422] POST http://localhost:8080/webhook [evt_fdsa]

The incoming message looked like this (but as a string). The event payload was also formatted as a string, but I changed it to the JSON format for the better readability.

{
  "type": "webhook_event",
  "endpoint": { "api_version": "2025-01-27.acacia" },
  "event_payload": "<see below in collapsed section>",
  "http_headers": {
    "Accept": "*/*; q=0.5, application/xml",
    "Cache-Control": "no-cache",
    "Content-Type": "application/json; charset=utf-8",
    "Stripe-Signature": "t=1233,v1=asdf1234,v0=asdf1234",
    "User-Agent": "Stripe/1.0 (+https://stripe.com/docs/webhooks)"
  },
  "webhook_conversation_id": "123-wc_asdf",
  "webhook_id": "1234-wh_asdf"
}
Whole payload
{
  "id": "evt_asdf",
  "object": "event",
  "api_version": "2025-01-27.acacia",
  "created": 1738350837,
  "data": {
    "object": {
      "id": "ch_asdf",
      "object": "charge",
      "amount": 2000,
      "amount_captured": 2000,
      "amount_refunded": 0,
      "application": null,
      "application_fee": null,
      "application_fee_amount": null,
      "balance_transaction": "txn_asdf",
      "billing_details": {
        "address": {
          "city": null,
          "country": null,
          "line1": null,
          "line2": null,
          "postal_code": null,
          "state": null
        },
        "email": null,
        "name": null,
        "phone": null
      },
      "calculated_statement_descriptor": "ACME",
      "captured": true,
      "created": 1738350837,
      "currency": "usd",
      "customer": null,
      "description": "(created by Stripe CLI)",
      "destination": null,
      "dispute": null,
      "disputed": false,
      "failure_balance_transaction": null,
      "failure_code": null,
      "failure_message": null,
      "fraud_details": {
      },
      "invoice": null,
      "livemode": false,
      "metadata": {
      },
      "on_behalf_of": null,
      "order": null,
      "outcome": {
        "advice_code": null,
        "network_advice_code": null,
        "network_decline_code": null,
        "network_status": "approved_by_network",
        "reason": null,
        "risk_level": "normal",
        "risk_score": 54,
        "seller_message": "Payment complete.",
        "type": "authorized"
      },
      "paid": true,
      "payment_intent": "pi_asdf",
      "payment_method": "pm_asdf",
      "payment_method_details": {
        "card": {
          "amount_authorized": 2000,
          "authorization_code": null,
          "brand": "visa",
          "checks": {
            "address_line1_check": null,
            "address_postal_code_check": null,
            "cvc_check": "pass"
          },
          "country": "US",
          "exp_month": 1,
          "exp_year": 2026,
          "extended_authorization": {
            "status": "disabled"
          },
          "fingerprint": "asdf",
          "funding": "credit",
          "incremental_authorization": {
            "status": "unavailable"
          },
          "installments": null,
          "last4": "4242",
          "mandate": null,
          "multicapture": {
            "status": "unavailable"
          },
          "network": "visa",
          "network_token": {
            "used": false
          },
          "network_transaction_id": "12345",
          "overcapture": {
            "maximum_amount_capturable": 2000,
            "status": "unavailable"
          },
          "regulated_status": "unregulated",
          "three_d_secure": null,
          "wallet": null
        },
        "type": "card"
      },
      "radar_options": {
      },
      "receipt_email": null,
      "receipt_number": null,
      "receipt_url": "https://pay.stripe.com/receipts/payment/asdf",
      "refunded": false,
      "review": null,
      "shipping": {
        "address": {
          "city": "San Francisco",
          "country": "US",
          "line1": "510 Townsend St",
          "line2": null,
          "postal_code": "94103",
          "state": "CA"
        },
        "carrier": null,
        "name": "Jenny Rosen",
        "phone": null,
        "tracking_number": null
      },
      "source": null,
      "source_transfer": null,
      "statement_descriptor": null,
      "statement_descriptor_suffix": null,
      "status": "succeeded",
      "transfer_data": null,
      "transfer_group": null
    }
  },
  "livemode": false,
  "pending_webhooks": 2,
  "request": {
    "id": "req_asdf",
    "idempotency_key": "1234567890-asdf-asdf-asdf-asdf1234"
  },
  "type": "charge.succeeded"
}

Since I can't see the response in the CLI, I sent the payload manually with Postman to http://localhost:8080/webhook where I got HTTP 422 and the message expected required property account to be present.

Environment:
stripe --version -> stripe version 1.23.10
go version -> go version go1.21.3 darwin/arm64
MacOS 15.2

@mbroshi-stripe
Copy link
Contributor

@m-mattia-m Did you try replacing StripeWebhookEventRequest with stripe.Event in these lines: https://github.com/m-mattia-m/stripe-go-event-bug-report/blob/main/main.go#L42-L43? When I do that I get 204 responses from the webhook.

@m-mattia-m
Copy link
Author

m-mattia-m commented Feb 1, 2025

@mbroshi-stripe, You are right. I got a 204 too after replacing the input structs. However, when I checked the incoming input struct with the debugger, it's an empty object. I also get the same output after changing StripeWebhookEventRequest to a struct like this:

type MyRandomTestStruct struct {
	firstname string `json:"firstname"`
	lastname  string `json:"lastname"`
}

Normally you define incoming requests in Huma with a body if you want to send a payload. For this reason, I think Huma treats every request like this because they don't have the expected attributes like headers, body, ... that Huma wants to parse.

Huma - Request Inputs

I think my demo repository handles the correct behavior from Huma which throws an error because the payload doesn't fulfill all the requirements.

@mbroshi-stripe
Copy link
Contributor

Thanks @m-mattia-m for the explanation! I was not familiar with Huma, but I see that it is opinionated on how it validates JSON: https://huma.rocks/features/request-validation/#optional-required. We do not have any plans at the moment to support Huma, so I would recommend using a design to handle incoming webhooks like the example here: https://github.com/stripe-samples/checkout-one-time-payments/blob/main/server/go/server.go#L131-L155.

@m-mattia-m
Copy link
Author

@mbroshi-stripe I totally understand if Huma isn't officially supported at the moment, but I wanted to highlight that this seems like a general issue rather than something specific to Huma, since the standard go library works the same way.

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string. Source: pkg.go.dev

The core issue is that JSON attributes are not required by default, whereas Go structures expect all attributes unless specified otherwise. As a result, attributes that aren't required should be explicitly marked as optional in Go.

For instance, the account attribute is defined as a "nullable string." If I understand correctly, your API omits nullable objects, meaning this attribute isn't included in the JSON. However, your SDK treats it as required. Adding omitempty to nullable objects/attributes would align the SDK’s behavior with your API documentation.

Let me know if I'm misunderstanding anything here - just wanted to bring this up for discussion. :)

@mbroshi-stripe
Copy link
Contributor

To be clear, omitempty is applicable to JSON encoding of Stripe objects, which is not part our API surface (our API inputs are form-encoded). Relying on JSON encoding for production behavior is not something we currently support.

That said, I've also in the past considered adding omitempty for better debuggability (rather than sifting through lots of empty JSON fields in log files). If it makes using some 3rd party libraries easier, all the better. I'll rename the Issue to reflect that ask.

@mbroshi-stripe mbroshi-stripe changed the title Optional attributes at the Event object are not optional Add omitempty to JSON field tags on optional fields Feb 3, 2025
@m-mattia-m
Copy link
Author

Ok sorry, I've never checked the content type because I thought since the payload is in JSON format it must be ⁠ application/json ⁠. However, I'm glad that you will consider this request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants