The Inbox: WebClient-Core-ct.124.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: WebClient-Core-ct.124.mcz

commits-2
Christoph Thiede uploaded a new version of WebClient-Core to project The Inbox:
http://source.squeak.org/inbox/WebClient-Core-ct.124.mcz

==================== Summary ====================

Name: WebClient-Core-ct.124
Author: ct
Time: 31 August 2020, 1:54:11.685896 am
UUID: b0d7790c-c195-6e4d-ad0f-fce8cf8b3b00
Ancestors: WebClient-Core-ul.123

Fixes a syntax error in multipart/form-data encoding

Phew! It costed me a few hours to track some higher-level application bug down to this low level code ...

=============== Diff against WebClient-Core-ul.123 ===============

Item was changed:
  ----- Method: WebUtils class>>encodeMultipartForm:boundary: (in category 'decoding') -----
  encodeMultipartForm: fieldMap boundary: boundary
  "Encodes the fieldMap as multipart/form-data.
 
  The fieldMap may contain MIMEDocument instances to indicate the presence
  of a file to upload to the server. If the MIMEDocument is present, its
  content type and file name will be used for the upload.
 
  The fieldMap can be EITHER an array of associations OR a Dictionary of
  key value pairs (the former is useful for providing multiple fields and/or
  specifying the order of fields)."
 
  ^String streamContents:[:stream|
  (fieldMap as: Dictionary) keysAndValuesDo:[:fieldName :fieldValue | | fieldContent |
  "Write multipart boundary and common headers"
  stream nextPutAll: '--', boundary; crlf.
  stream nextPutAll: 'Content-Disposition: form-data; name="', fieldName, '"'.
  "Figure out if this is a file upload"
  (fieldValue isKindOf: MIMEDocument) ifTrue:[
+ stream nextPutAll: '; filename="', fieldValue url pathForFile, '"'; crlf.
- stream nextPutAll: ' filename="', fieldValue url pathForFile, '"'; crlf.
  stream nextPutAll: 'Content-Type: ', fieldValue contentType.
  fieldContent := (fieldValue content ifNil:[
  (FileStream readOnlyFileNamed: fieldValue url pathForFile) contentsOfEntireFile.
  ]) asString.
  ] ifFalse: [fieldContent := fieldValue].
  stream crlf; crlf.
  stream nextPutAll: fieldContent asString.
  stream crlf.
  ].
  stream nextPutAll: '--', boundary, '--', String crlf.
  ].
  !


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: WebClient-Core-ct.124.mcz

Chris Muller-3
I'm not qualified to review the change but good dig!   :)

On Sun, Aug 30, 2020 at 6:54 PM <[hidden email]> wrote:

>
> Christoph Thiede uploaded a new version of WebClient-Core to project The Inbox:
> http://source.squeak.org/inbox/WebClient-Core-ct.124.mcz
>
> ==================== Summary ====================
>
> Name: WebClient-Core-ct.124
> Author: ct
> Time: 31 August 2020, 1:54:11.685896 am
> UUID: b0d7790c-c195-6e4d-ad0f-fce8cf8b3b00
> Ancestors: WebClient-Core-ul.123
>
> Fixes a syntax error in multipart/form-data encoding
>
> Phew! It costed me a few hours to track some higher-level application bug down to this low level code ...
>
> =============== Diff against WebClient-Core-ul.123 ===============
>
> Item was changed:
>   ----- Method: WebUtils class>>encodeMultipartForm:boundary: (in category 'decoding') -----
>   encodeMultipartForm: fieldMap boundary: boundary
>         "Encodes the fieldMap as multipart/form-data.
>
>         The fieldMap may contain MIMEDocument instances to indicate the presence
>         of a file to upload to the server. If the MIMEDocument is present, its
>         content type and file name will be used for the upload.
>
>         The fieldMap can be EITHER an array of associations OR a Dictionary of
>         key value pairs (the former is useful for providing multiple fields and/or
>         specifying the order of fields)."
>
>         ^String streamContents:[:stream|
>                 (fieldMap as: Dictionary) keysAndValuesDo:[:fieldName :fieldValue | | fieldContent |
>                         "Write multipart boundary and common headers"
>                         stream nextPutAll: '--', boundary; crlf.
>                         stream nextPutAll: 'Content-Disposition: form-data; name="', fieldName, '"'.
>                         "Figure out if this is a file upload"
>                         (fieldValue isKindOf: MIMEDocument) ifTrue:[
> +                               stream nextPutAll: '; filename="', fieldValue url pathForFile, '"'; crlf.
> -                               stream nextPutAll: ' filename="', fieldValue url pathForFile, '"'; crlf.
>                                 stream nextPutAll: 'Content-Type: ', fieldValue contentType.
>                                 fieldContent := (fieldValue content ifNil:[
>                                         (FileStream readOnlyFileNamed: fieldValue url pathForFile) contentsOfEntireFile.
>                                 ]) asString.
>                         ] ifFalse: [fieldContent := fieldValue].
>                         stream crlf; crlf.
>                         stream nextPutAll: fieldContent asString.
>                         stream crlf.
>                 ].
>                 stream nextPutAll: '--', boundary, '--', String crlf.
>         ].
>   !
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: WebClient-Core-ct.124.mcz

Levente Uzonyi
The change looks good to me. It would be better if we had a test case
covering this method.


Levente

On Mon, 31 Aug 2020, Chris Muller wrote:

> I'm not qualified to review the change but good dig!   :)
>
> On Sun, Aug 30, 2020 at 6:54 PM <[hidden email]> wrote:
>>
>> Christoph Thiede uploaded a new version of WebClient-Core to project The Inbox:
>> http://source.squeak.org/inbox/WebClient-Core-ct.124.mcz
>>
>> ==================== Summary ====================
>>
>> Name: WebClient-Core-ct.124
>> Author: ct
>> Time: 31 August 2020, 1:54:11.685896 am
>> UUID: b0d7790c-c195-6e4d-ad0f-fce8cf8b3b00
>> Ancestors: WebClient-Core-ul.123
>>
>> Fixes a syntax error in multipart/form-data encoding
>>
>> Phew! It costed me a few hours to track some higher-level application bug down to this low level code ...
>>
>> =============== Diff against WebClient-Core-ul.123 ===============
>>
>> Item was changed:
>>   ----- Method: WebUtils class>>encodeMultipartForm:boundary: (in category 'decoding') -----
>>   encodeMultipartForm: fieldMap boundary: boundary
>>         "Encodes the fieldMap as multipart/form-data.
>>
>>         The fieldMap may contain MIMEDocument instances to indicate the presence
>>         of a file to upload to the server. If the MIMEDocument is present, its
>>         content type and file name will be used for the upload.
>>
>>         The fieldMap can be EITHER an array of associations OR a Dictionary of
>>         key value pairs (the former is useful for providing multiple fields and/or
>>         specifying the order of fields)."
>>
>>         ^String streamContents:[:stream|
>>                 (fieldMap as: Dictionary) keysAndValuesDo:[:fieldName :fieldValue | | fieldContent |
>>                         "Write multipart boundary and common headers"
>>                         stream nextPutAll: '--', boundary; crlf.
>>                         stream nextPutAll: 'Content-Disposition: form-data; name="', fieldName, '"'.
>>                         "Figure out if this is a file upload"
>>                         (fieldValue isKindOf: MIMEDocument) ifTrue:[
>> +                               stream nextPutAll: '; filename="', fieldValue url pathForFile, '"'; crlf.
>> -                               stream nextPutAll: ' filename="', fieldValue url pathForFile, '"'; crlf.
>>                                 stream nextPutAll: 'Content-Type: ', fieldValue contentType.
>>                                 fieldContent := (fieldValue content ifNil:[
>>                                         (FileStream readOnlyFileNamed: fieldValue url pathForFile) contentsOfEntireFile.
>>                                 ]) asString.
>>                         ] ifFalse: [fieldContent := fieldValue].
>>                         stream crlf; crlf.
>>                         stream nextPutAll: fieldContent asString.
>>                         stream crlf.
>>                 ].
>>                 stream nextPutAll: '--', boundary, '--', String crlf.
>>         ].
>>   !
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: WebClient-Core-ct.124.mcz

Christoph Thiede

Hi Levente,


thanks for the feedback. We already have WebClientServerTest >> #testMultipartFiles but it already passed before this patch because the decoding implementation in WebUtils (#decodeMultipartForm:boundary:do:) is very robust: Instead of parsing a specific header syntax, it just searches for patterns matching #=, just see yourself ...


I did not want to touch this because I thought robust client/server implementations are a good thing in general (at least every web browser is able to display malformed HTML pages), but this hinders us from writing simple integration tests. Would you recommend to sharpen the decoding implementation or rather to write separate unit tests? :-)


Best,

Christoph


Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]>
Gesendet: Dienstag, 1. September 2020 16:18:53
An: [hidden email]; The general-purpose Squeak developers list
Betreff: Re: [squeak-dev] The Inbox: WebClient-Core-ct.124.mcz
 
The change looks good to me. It would be better if we had a test case
covering this method.


Levente

On Mon, 31 Aug 2020, Chris Muller wrote:

> I'm not qualified to review the change but good dig!   :)
>
> On Sun, Aug 30, 2020 at 6:54 PM <[hidden email]> wrote:
>>
>> Christoph Thiede uploaded a new version of WebClient-Core to project The Inbox:
>> http://source.squeak.org/inbox/WebClient-Core-ct.124.mcz
>>
>> ==================== Summary ====================
>>
>> Name: WebClient-Core-ct.124
>> Author: ct
>> Time: 31 August 2020, 1:54:11.685896 am
>> UUID: b0d7790c-c195-6e4d-ad0f-fce8cf8b3b00
>> Ancestors: WebClient-Core-ul.123
>>
>> Fixes a syntax error in multipart/form-data encoding
>>
>> Phew! It costed me a few hours to track some higher-level application bug down to this low level code ...
>>
>> =============== Diff against WebClient-Core-ul.123 ===============
>>
>> Item was changed:
>>   ----- Method: WebUtils class>>encodeMultipartForm:boundary: (in category 'decoding') -----
>>   encodeMultipartForm: fieldMap boundary: boundary
>>         "Encodes the fieldMap as multipart/form-data.
>>
>>         The fieldMap may contain MIMEDocument instances to indicate the presence
>>         of a file to upload to the server. If the MIMEDocument is present, its
>>         content type and file name will be used for the upload.
>>
>>         The fieldMap can be EITHER an array of associations OR a Dictionary of
>>         key value pairs (the former is useful for providing multiple fields and/or
>>         specifying the order of fields)."
>>
>>         ^String streamContents:[:stream|
>>                 (fieldMap as: Dictionary) keysAndValuesDo:[:fieldName :fieldValue | | fieldContent |
>>                         "Write multipart boundary and common headers"
>>                         stream nextPutAll: '--', boundary; crlf.
>>                         stream nextPutAll: 'Content-Disposition: form-data; name="', fieldName, '"'.
>>                         "Figure out if this is a file upload"
>>                         (fieldValue isKindOf: MIMEDocument) ifTrue:[
>> +                               stream nextPutAll: '; filename="', fieldValue url pathForFile, '"'; crlf.
>> -                               stream nextPutAll: ' filename="', fieldValue url pathForFile, '"'; crlf.
>>                                 stream nextPutAll: 'Content-Type: ', fieldValue contentType.
>>                                 fieldContent := (fieldValue content ifNil:[
>>                                         (FileStream readOnlyFileNamed: fieldValue url pathForFile) contentsOfEntireFile.
>>                                 ]) asString.
>>                         ] ifFalse: [fieldContent := fieldValue].
>>                         stream crlf; crlf.
>>                         stream nextPutAll: fieldContent asString.
>>                         stream crlf.
>>                 ].
>>                 stream nextPutAll: '--', boundary, '--', String crlf.
>>         ].
>>   !
>>
>>



Carpe Squeak!