| Message ID | 20230502092242.63439-1-christian.storm@siemens.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [V2] channel_curl: Set charset in Content-Type Header | expand |
Hi Christian, On 02.05.23 11:22, 'Christian Storm' via swupdate wrote: > Replace 'charsets: utf-8' HTTP Header by 'Content-Type:' Header's > 'charset=' parameter: > As per RFC 4627, the default encoding for application/json is UTF-8. > RFC 8259 demands that JSON text exchanged between non-closed systems > must be UTF-8 encoded. It continues to state that no 'charset' > parameter is defined for application/json. Hence, while adding one > has no effect on compliant recipients according to RFC 8259, do not > add a non-defined parameter. > > This also fixes adding the Header 'charsets: utf-8' twice for > application/json and application/text content if no memory > allocation error occurs. > I have not understood, apart to be compliant with RFC, if there is a real bug or communication issue when request is sent, ist it ? > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > Rebased to current HEAD, didn't apply cleanly there. > Changed "Set channel header Accept failed." to "Setting channel header Accept failed." for symmetry. > > > corelib/channel_curl.c | 47 ++++++++++++++---------------------------- > 1 file changed, 16 insertions(+), 31 deletions(-) > > diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > index f12ff7be..2bd23571 100644 > --- a/corelib/channel_curl.c > +++ b/corelib/channel_curl.c > @@ -521,41 +521,26 @@ static channel_op_res_t channel_set_content_type(channel_t *this, > else > content = "application/json"; > > - if (ENOMEM_ASPRINTF == > - asprintf(&contenttype, "Content-Type: %s", > - content)) { > - result = CHANNEL_EINIT; > + if (ENOMEM_ASPRINTF == asprintf(&contenttype, "Content-Type: %s%s", content, > + !strcmp(content, "application/text") ? "; charset=utf-8" : "")) { > ERROR("OOM when setting Content-type."); The only change is that now there is no charset if content is application/text. Is this wanted or better should we still rely to hard-code the check ? > - } > - > - if (ENOMEM_ASPRINTF == > - asprintf(&accept, "Accept: %s", > - content)) { > - result = CHANNEL_EINIT; > - ERROR("OOM when setting Accept."); > - } > - > - if (result == CHANNEL_OK) { > - if (((channel_curl->header = curl_slist_append( > - channel_curl->header, contenttype)) == > - NULL) || > - ((channel_curl->header = curl_slist_append( > - channel_curl->header, accept)) == NULL) || > - ((channel_curl->header = curl_slist_append( > - channel_curl->header, "charsets: utf-8")) == NULL)) { > - ERROR("Set channel header failed."); > result = CHANNEL_EINIT; > + } else { > + if ((channel_curl->header = curl_slist_append(channel_curl->header, > + contenttype)) == NULL) { > + ERROR("Setting channel header Content-type failed."); > + result = CHANNEL_EINIT; > } > } > - /* > - * Add default charset for application content > - */ > - if ((!strcmp(content, "application/json") || !strcmp(content, "application/text")) && > - (result == CHANNEL_OK)) { > - if ((channel_curl->header = curl_slist_append( > - channel_curl->header, "charsets: utf-8")) == NULL) { > - ERROR("Set channel charset header failed."); > - result = CHANNEL_EINIT; > + > + if (ENOMEM_ASPRINTF == asprintf(&accept, "Accept: %s", content)) { > + ERROR("OOM when setting Accept."); > + result = CHANNEL_EINIT; > + } else { > + if ((channel_curl->header = curl_slist_append(channel_curl->header, > + accept)) == NULL) { > + ERROR("Setting channel header Accept failed."); > + result = CHANNEL_EINIT; > } > } > Best regards, Stefano
Hi Stefano, > > Replace 'charsets: utf-8' HTTP Header by 'Content-Type:' Header's > > 'charset=' parameter: > > As per RFC 4627, the default encoding for application/json is UTF-8. > > RFC 8259 demands that JSON text exchanged between non-closed systems > > must be UTF-8 encoded. It continues to state that no 'charset' > > parameter is defined for application/json. Hence, while adding one > > has no effect on compliant recipients according to RFC 8259, do not > > add a non-defined parameter. > > > > This also fixes adding the Header 'charsets: utf-8' twice for > > application/json and application/text content if no memory > > allocation error occurs. > > > > I have not understood, apart to be compliant with RFC, if there is a real bug > or communication issue when request is sent, ist it ? No, not a real bug but a backend bugging me that I sent a Header ('charsets: utf-8') twice. This is why I did take a look into it. Then, I looked up the RFCs as it also complained about sending a charset= with JSON content for which it isn't defined. So, no, not a bug but more a "compliance fix". > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > --- > > Rebased to current HEAD, didn't apply cleanly there. > > Changed "Set channel header Accept failed." to "Setting channel header Accept failed." for symmetry. > > > > > > corelib/channel_curl.c | 47 ++++++++++++++---------------------------- > > 1 file changed, 16 insertions(+), 31 deletions(-) > > > > diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c > > index f12ff7be..2bd23571 100644 > > --- a/corelib/channel_curl.c > > +++ b/corelib/channel_curl.c > > @@ -521,41 +521,26 @@ static channel_op_res_t channel_set_content_type(channel_t *this, > > else > > content = "application/json"; > > - if (ENOMEM_ASPRINTF == > > - asprintf(&contenttype, "Content-Type: %s", > > - content)) { > > - result = CHANNEL_EINIT; > > + if (ENOMEM_ASPRINTF == asprintf(&contenttype, "Content-Type: %s%s", content, > > + !strcmp(content, "application/text") ? "; charset=utf-8" : "")) { > > ERROR("OOM when setting Content-type."); > > The only change is that now there is no charset if content is > application/text. Is this wanted or better should we still rely to hard-code > the check ? Hm, !strcmp("application/text", "application/text") gives true, so to append charset=utf-8 for application/text and not for other content, in particular not for the default application/json as per the RFC cited. > > - } > > - > > - if (ENOMEM_ASPRINTF == > > - asprintf(&accept, "Accept: %s", > > - content)) { > > - result = CHANNEL_EINIT; > > - ERROR("OOM when setting Accept."); > > - } > > - > > - if (result == CHANNEL_OK) { > > - if (((channel_curl->header = curl_slist_append( > > - channel_curl->header, contenttype)) == > > - NULL) || > > - ((channel_curl->header = curl_slist_append( > > - channel_curl->header, accept)) == NULL) || > > - ((channel_curl->header = curl_slist_append( > > - channel_curl->header, "charsets: utf-8")) == NULL)) { > > - ERROR("Set channel header failed."); > > result = CHANNEL_EINIT; > > + } else { > > + if ((channel_curl->header = curl_slist_append(channel_curl->header, > > + contenttype)) == NULL) { > > + ERROR("Setting channel header Content-type failed."); > > + result = CHANNEL_EINIT; > > } > > } > > - /* > > - * Add default charset for application content > > - */ > > - if ((!strcmp(content, "application/json") || !strcmp(content, "application/text")) && > > - (result == CHANNEL_OK)) { > > - if ((channel_curl->header = curl_slist_append( > > - channel_curl->header, "charsets: utf-8")) == NULL) { > > - ERROR("Set channel charset header failed."); > > - result = CHANNEL_EINIT; > > + > > + if (ENOMEM_ASPRINTF == asprintf(&accept, "Accept: %s", content)) { > > + ERROR("OOM when setting Accept."); > > + result = CHANNEL_EINIT; > > + } else { > > + if ((channel_curl->header = curl_slist_append(channel_curl->header, > > + accept)) == NULL) { > > + ERROR("Setting channel header Accept failed."); > > + result = CHANNEL_EINIT; > > } > > } Kind regards, Christian
On 08.05.23 15:53, 'Christian Storm' via swupdate wrote: > Hi Stefano, > >>> Replace 'charsets: utf-8' HTTP Header by 'Content-Type:' Header's >>> 'charset=' parameter: >>> As per RFC 4627, the default encoding for application/json is UTF-8. >>> RFC 8259 demands that JSON text exchanged between non-closed systems >>> must be UTF-8 encoded. It continues to state that no 'charset' >>> parameter is defined for application/json. Hence, while adding one >>> has no effect on compliant recipients according to RFC 8259, do not >>> add a non-defined parameter. >>> >>> This also fixes adding the Header 'charsets: utf-8' twice for >>> application/json and application/text content if no memory >>> allocation error occurs. >>> >> >> I have not understood, apart to be compliant with RFC, if there is a real bug >> or communication issue when request is sent, ist it ? > > No, not a real bug but a backend bugging me that I sent a Header > ('charsets: utf-8') twice. This is why I did take a look into it. > Then, I looked up the RFCs as it also complained about sending a > charset= with JSON content for which it isn't defined. > So, no, not a bug but more a "compliance fix". > > > >>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>> --- >>> Rebased to current HEAD, didn't apply cleanly there. >>> Changed "Set channel header Accept failed." to "Setting channel header Accept failed." for symmetry. >>> >>> >>> corelib/channel_curl.c | 47 ++++++++++++++---------------------------- >>> 1 file changed, 16 insertions(+), 31 deletions(-) >>> >>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c >>> index f12ff7be..2bd23571 100644 >>> --- a/corelib/channel_curl.c >>> +++ b/corelib/channel_curl.c >>> @@ -521,41 +521,26 @@ static channel_op_res_t channel_set_content_type(channel_t *this, >>> else >>> content = "application/json"; >>> - if (ENOMEM_ASPRINTF == >>> - asprintf(&contenttype, "Content-Type: %s", >>> - content)) { >>> - result = CHANNEL_EINIT; >>> + if (ENOMEM_ASPRINTF == asprintf(&contenttype, "Content-Type: %s%s", content, >>> + !strcmp(content, "application/text") ? "; charset=utf-8" : "")) { >>> ERROR("OOM when setting Content-type."); >> >> The only change is that now there is no charset if content is >> application/text. Is this wanted or better should we still rely to hard-code >> the check ? > > Hm, !strcmp("application/text", "application/text") gives true, Yes, sorry, I wrote the opposite. > so to > append charset=utf-8 for application/text and not for other content, in > particular not for the default application/json as per the RFC cited. Agree that behavior is not changed for not application/json. I apply this - it could be that in future we need to be more flexible and not bind charset even with application/test, but for now it is fine. Best regards, Stefano > > > >>> - } >>> - >>> - if (ENOMEM_ASPRINTF == >>> - asprintf(&accept, "Accept: %s", >>> - content)) { >>> - result = CHANNEL_EINIT; >>> - ERROR("OOM when setting Accept."); >>> - } >>> - >>> - if (result == CHANNEL_OK) { >>> - if (((channel_curl->header = curl_slist_append( >>> - channel_curl->header, contenttype)) == >>> - NULL) || >>> - ((channel_curl->header = curl_slist_append( >>> - channel_curl->header, accept)) == NULL) || >>> - ((channel_curl->header = curl_slist_append( >>> - channel_curl->header, "charsets: utf-8")) == NULL)) { >>> - ERROR("Set channel header failed."); >>> result = CHANNEL_EINIT; >>> + } else { >>> + if ((channel_curl->header = curl_slist_append(channel_curl->header, >>> + contenttype)) == NULL) { >>> + ERROR("Setting channel header Content-type failed."); >>> + result = CHANNEL_EINIT; >>> } >>> } >>> - /* >>> - * Add default charset for application content >>> - */ >>> - if ((!strcmp(content, "application/json") || !strcmp(content, "application/text")) && >>> - (result == CHANNEL_OK)) { >>> - if ((channel_curl->header = curl_slist_append( >>> - channel_curl->header, "charsets: utf-8")) == NULL) { >>> - ERROR("Set channel charset header failed."); >>> - result = CHANNEL_EINIT; >>> + >>> + if (ENOMEM_ASPRINTF == asprintf(&accept, "Accept: %s", content)) { >>> + ERROR("OOM when setting Accept."); >>> + result = CHANNEL_EINIT; >>> + } else { >>> + if ((channel_curl->header = curl_slist_append(channel_curl->header, >>> + accept)) == NULL) { >>> + ERROR("Setting channel header Accept failed."); >>> + result = CHANNEL_EINIT; >>> } >>> } > > Kind regards, > Christian >
diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c index f12ff7be..2bd23571 100644 --- a/corelib/channel_curl.c +++ b/corelib/channel_curl.c @@ -521,41 +521,26 @@ static channel_op_res_t channel_set_content_type(channel_t *this, else content = "application/json"; - if (ENOMEM_ASPRINTF == - asprintf(&contenttype, "Content-Type: %s", - content)) { - result = CHANNEL_EINIT; + if (ENOMEM_ASPRINTF == asprintf(&contenttype, "Content-Type: %s%s", content, + !strcmp(content, "application/text") ? "; charset=utf-8" : "")) { ERROR("OOM when setting Content-type."); - } - - if (ENOMEM_ASPRINTF == - asprintf(&accept, "Accept: %s", - content)) { - result = CHANNEL_EINIT; - ERROR("OOM when setting Accept."); - } - - if (result == CHANNEL_OK) { - if (((channel_curl->header = curl_slist_append( - channel_curl->header, contenttype)) == - NULL) || - ((channel_curl->header = curl_slist_append( - channel_curl->header, accept)) == NULL) || - ((channel_curl->header = curl_slist_append( - channel_curl->header, "charsets: utf-8")) == NULL)) { - ERROR("Set channel header failed."); result = CHANNEL_EINIT; + } else { + if ((channel_curl->header = curl_slist_append(channel_curl->header, + contenttype)) == NULL) { + ERROR("Setting channel header Content-type failed."); + result = CHANNEL_EINIT; } } - /* - * Add default charset for application content - */ - if ((!strcmp(content, "application/json") || !strcmp(content, "application/text")) && - (result == CHANNEL_OK)) { - if ((channel_curl->header = curl_slist_append( - channel_curl->header, "charsets: utf-8")) == NULL) { - ERROR("Set channel charset header failed."); - result = CHANNEL_EINIT; + + if (ENOMEM_ASPRINTF == asprintf(&accept, "Accept: %s", content)) { + ERROR("OOM when setting Accept."); + result = CHANNEL_EINIT; + } else { + if ((channel_curl->header = curl_slist_append(channel_curl->header, + accept)) == NULL) { + ERROR("Setting channel header Accept failed."); + result = CHANNEL_EINIT; } }
Replace 'charsets: utf-8' HTTP Header by 'Content-Type:' Header's 'charset=' parameter: As per RFC 4627, the default encoding for application/json is UTF-8. RFC 8259 demands that JSON text exchanged between non-closed systems must be UTF-8 encoded. It continues to state that no 'charset' parameter is defined for application/json. Hence, while adding one has no effect on compliant recipients according to RFC 8259, do not add a non-defined parameter. This also fixes adding the Header 'charsets: utf-8' twice for application/json and application/text content if no memory allocation error occurs. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- Rebased to current HEAD, didn't apply cleanly there. Changed "Set channel header Accept failed." to "Setting channel header Accept failed." for symmetry. corelib/channel_curl.c | 47 ++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 31 deletions(-)