VictorAvelar / mollie-api-go

Golang wrapper for Mollie's REST API with full resource coverage.
https://www.mollie.com/developers/libraries/golang
MIT License
63 stars 37 forks source link

escaping the path in URL's #150

Closed Dynom closed 2 years ago

Dynom commented 3 years ago

Hi,

I've noticed that on a few places you've chosen to add arguments to the URL, without adding any URL escaping. This unfortunately forces the consumer of e.g.: mollie.SubscriptionService{} to know about the underlying transport mechanism and requires them to apply URL escaping prior to calling a service, i.e.:

func (p *provider) CancelRecurringPayment(customerID UserID, subscriptionID string) error {

    // Note: github.com/VictorAvelar/mollie-api-go/v2/mollie does not escape these arguments, so we do it here.
    customerID = url.PathEscape(customerID)
    subscriptionID = url.PathEscape(subscriptionID)

    _, err := p.client.Subscriptions.Delete(customerID, subscriptionID)

    return err
}

I realise that the Mollie identifiers commonly don't contain reserved characters, but that's a lof of trust to expect for user of the SDK :-)

Other locations ```text mollie/captures.go 51: u := fmt.Sprintf("v2/payments/%s/captures/%s", pID, cID) 72: u := fmt.Sprintf("v2/payments/%s/captures", pID) mollie/chargebacks.go 63: u := fmt.Sprintf("v2/payments/%s/chargebacks/%s", paymentID, chargebackID) 98: u := fmt.Sprintf("v2/payments/%s/chargebacks", paymentID) mollie/customers.go 163: u := fmt.Sprintf("v2/customers/%s/payments", id) 184: u := fmt.Sprintf("v2/customers/%s/payments", id) mollie/profiles.go 170: u := fmt.Sprintf("v2/profiles/%s/methods/%s", id, pm) 188: u := fmt.Sprintf("v2/profiles/%s/methods/%s", id, pm) 257: u := fmt.Sprintf("v2/profiles/%s/methods/giftcard/issuers/%s", profileID, issuer) mollie/mandates.go 104: u := fmt.Sprintf("v2/customers/%s/mandates", cID) 128: u := fmt.Sprintf("v2/customers/%s/mandates/%s", cID, mID) 151: u := fmt.Sprintf("v2/customers/%s/mandates/%s", cID, mID) 170: u := fmt.Sprintf("v2/customers/%s/mandates", cID) mollie/shipments.go 46: u := fmt.Sprintf("v2/orders/%s/shipments/%s", oID, sID) 74: u := fmt.Sprintf("v2/orders/%s/shipments", oID) 109: u := fmt.Sprintf("v2/orders/%s/shipments", oID) 130: u := fmt.Sprintf("v2/orders/%s/shipments/%s", oID, sID) mollie/subscriptions.go 79: u := fmt.Sprintf("v2/customers/%s/subscriptions/%s", cID, sID) 100: u := fmt.Sprintf("v2/customers/%s/subscriptions", cID) 126: u := fmt.Sprintf("v2/customers/%s/subscriptions/%s", cID, sID) 148: u := fmt.Sprintf("v2/customers/%s/subscriptions/%s", cID, sID) 193: u := fmt.Sprintf("v2/customers/%s/subscriptions", cID) 215: u := fmt.Sprintf("v2/customers/%s/subscriptions/%s/payments", cID, sID) mollie/refunds.go 85: u := fmt.Sprintf("v2/payments/%s/refunds/%s", paymentID, refundID) 112: u := fmt.Sprintf("v2/payments/%s/refunds", paymentID) 143: u := fmt.Sprintf("v2/payments/%s/refunds/%s", paymentID, refundID) 179: u := fmt.Sprintf("v2/payments/%s/refunds", paymentID) mollie/orders.go 348: u := fmt.Sprintf("v2/orders/%s/lines/%s", orderID, orderLineID) 373: u := fmt.Sprintf("v2/orders/%s/lines", orderID) 392: u := fmt.Sprintf("v2/orders/%s/payments", orderID) 414: u := fmt.Sprintf("v2/orders/%s/refunds", orderID) 436: u := fmt.Sprintf("v2/orders/%s/refunds", orderID) ```
VictorAvelar commented 3 years ago

Another valuable issue you are raising, I also think this one makes a lot of sense.

Thanks for the super detailed explanation 😄

Stuie commented 3 years ago

Can this be assigned to me so I can take a look at escaping all the mentioned places, please?

Stuie commented 3 years ago

I've created a draft PR. I've added some questions to the bottom.

I need to test the change on another computer. Docker is fighting me on my Windows machine. I can look to add tests once I know the approach is good.

VictorAvelar commented 3 years ago

I'll have a look

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

VictorAvelar commented 2 years ago

Just to bring some closure to the topic, after checking multiple similar clients I couldn't escaping the url path variables, I am not saying that the fact that it is not done makes it right, I am just against the complication and repetition it would bring to the library.

I appreciate your comments and the effort invested when describing the issue and possible solutions, hope you understand my reasoning, thanks!

Go github library example. Go Digitalocean library example. Go Jira API library example