Possible alternative to configQueryExtraUnreserved option?

[thomasjm]

haskell/kubernetes/lib/Kubernetes/OpenAPI/Client.hs

Line 187 in 94ee7cc

in if B.null queryExtraUnreserved

This option was introduced here: 541fb76.

I have been running commands using the pod "exec" endpoint, and some of my commands had plus signs in them. (Because of new-style Nix hashes, as it turns out.) I spent many hours scratching my head about why they weren't executing properly before I found this change.

I don't think it makes sense to set a global configQueryExtraUnreserved in the KubernetesClientConfig, because sometimes this kind of escaping needs to happen and sometimes it doesn't.

I was wondering if an alternative might be possible which is easier to use/more apparent when you're getting it wrong. I'm picturing a newtype called PartiallyUnescaped or UnescapePlusSigns or something, which can be wrapped around a normal query param, so you could do something like

let req = connectGetNamespacedPodExec (Accept MimeAny) (Name podName) namespace 
      -&- (UnescapePlusSigns (SomeFlag "environment+in"))

and then define the toQueryParam method of ToHttpApiData (UnescapePlusSigns a) accordingly.

I'm not married to any particular solution of course. CC @jonschoning

[jonschoning]

Before addressing the proposed solution, I just want to be clear about how the code generator works.

---

From the point of view of the generator api, technically -&- applies an "optional param" which is up to the operation to decide how that optional param is encoded. it could be a query param, but also be header or multiform data, etc.

The operation function only demands the required parameters as part of the function arguments (Name name) (Namespace namespace) and gives you the ability to supply optional parameters, hiding from you how they are included in the actual request.

Required parameters could also end up as query parameters for example

loginUser
  :: Accept accept -- ^ request accept ('MimeType')
  -> Username -- ^ "username" -  The user name for login
  -> Password -- ^ "password" -  The password for login in clear text
  -> OpenAPIPetstoreRequest LoginUser MimeNoContent Text accept
loginUser  _ (Username username) (Password password) =
  _mkRequest "GET" ["/user/login"]
    `addQuery` toQuery ("username", Just username)
    `addQuery` toQuery ("password", Just password)

---

So, we could introduce a change like you propose only for HasOptionalParam , (that is mostly what kubernetes uses), but that leaves any required query params unaddressed.

Alternatively we could come up with a method that works for required params too, however the required parameters are harder to change since they're tied to concrete types in the generated code.

Another option which would be to let the user modify the request object instead of the operation arguments, to tell it how those query params get plus+encoded, this would be a less-typed solution but more general.

A lot of this depends on how typed the fix needs to be. even for your example UnescapePlusSigns needs to have the appropriate instances to typecheck with the HasOptionalParam class.

I'll think about it and play around with some code to see the best way to provide a solution to this.

The only place i know of where plus-encoding is not allowed is in labels and selectors currently, but that seems like an important piece, so i'm not sure if its a better default to not plus-encode or default to plus-encode.

(if you're really stuck, you can always have more than one config object KubernetesClientConfig and use different configs or create a temporary modified config, depending on the request, but i realize that's not ideal.)

[thomasjm]

Thanks for the quick response!

FWIW, I have read that while there isn't good universal agreement about how plus signs in URLs should be treated, the W3C URI spec says the following:

Within the query string, the plus sign is reserved as shorthand notation for a space. Therefore, real plus signs must be encoded.

Other guidance I've seen for API designers is in line with this as well. Therefore, I'd suggest defaulting to plus-encode and treating the places where Kubernetes APIs violate this rule as the exceptional cases. Hopefully such cases are rare and limited to things like labels and selectors (I'm not aware of any others either).

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[jonschoning]

/remove-lifecycle stale

[hughjfchen]

Is this related to #67 , which has been fixed by PR #92 ?

[thomasjm]

Yes @hughjfchen, I referenced the relevant commit (541fb76) in the initial post. I'd say "fixed" is a strong word :P

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[jonschoning]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[jonschoning]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[jonschoning]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[thomasjm]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[thomasjm]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[thomasjm]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[thomasjm]

/remove-lifecycle stale