Fix handling of array form parameters #43

mkistler · 2019-09-15T16:02:10Z

This PR fixes the handling of array form parameters.

In a multipart/form-data request body, an array property should be serialized as multiple form data parts, each with the name of the array property and one of the elements of the array.

I've also added documentation for the primary exported method, createRequest, and refactored the processing of file properties to make it simpler/clearer.

mkistler · 2019-09-15T19:14:49Z

I modified the Watson Node SDK to use this version of the core and ran the full test suite, and that turned up one issue that needs further investigation. A STT test failed when calling addCorpus. The error response is:

Bad Request: Invalid corpus content type. Please use 'multipart/form-data' and 'corpus_file' key to upload the corpus file.

codecov · 2019-09-15T19:20:56Z

Codecov Report

Merging #43 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   93.06%   93.05%   -0.02%     
==========================================
  Files          12       12              
  Lines         519      518       -1     
  Branches      155      157       +2     
==========================================
- Hits          483      482       -1     
  Misses         35       35              
  Partials        1        1

Impacted Files	Coverage Δ
lib/base_service.ts	`91.66% <ø> (ø)`	⬆️
lib/requestwrapper.ts	`84.12% <100%> (-0.49%)`	⬇️
lib/helper.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15bce33...1680c2c. Read the comment docs.

mkistler · 2019-09-15T21:38:14Z

I have investigated the failure in the STT addCorpus test and it looks like there is a corner case that we have to work out. The failing STT test calls addCorpus as follows:

    speech_to_text.addCorpus(
      {
        customization_id: customization_id,
        corpus_name: 'test_corpus_2',
        corpus_file: fs
          .readFileSync(path.join(__dirname, '../resources/speech_to_text/corpus-short-2.txt'))
          .toString(),
        allow_overwrite: true,
      },
      done
    );

So the call is passing a plain string as the value for corpus_file. This parameter is defined in the SDK as:

 * @param {NodeJS.ReadableStream|FileObject|Buffer} params.corpus_file

So a string is technically not a valid input. But it worked before, so could we make it work now?

It seems that the way it worked before was that the STT SDK constructed a FileParamAttributes object and passed the string in the data property. That also isn't valid according to the node-sdk-core, which defines FileParamAttributes as:

export interface FileParamAttributes {
  data: NodeJS.ReadableStream | Buffer | FileObject;
  contentType: string;
  filename: string;
}

But it worked before ... so could we extend the definition of FileParamAttributes to allow a string to be passed in the data property? Well, if we did, we would break this test in "isFileParam.test.js":

  it('should not think object is file just because of `data` property', () => {
    const nonFileObject = { data: 'should not be interpreted as file' };
    expect(isFileParam(nonFileObject)).toBe(false);
  });

So ... what to do?

dpopp07

One change to request in the null/undefined filter and one other comment.

Not sure you if you want to include the removal of FileObject in this PR - if so, we should change the target branch to be release-candidate-v1.

dpopp07 · 2019-09-16T16:20:30Z

lib/requestwrapper.ts

-        }
+        const values = Array.isArray(formData[key]) ? formData[key] : [formData[key]];
+        // Skip keys with undefined/null values or empty object value
+        values.filter(v => v != null && !isEmptyObject(v)).forEach(value => {


I think this will miss values of explicit type undefined. The method isEmptyObject is pretty strict about returning true iff the value passed in is the empty object {}.

Something a little looser would catch more values:

Suggested change

values.filter(v => v != null && !isEmptyObject(v)).forEach(value => {

values.filter(v => v && !isEmptyObject(v)).forEach(value => {

Or we can just be more explicit:

values.filter(v => v != null && v != undefined && !isEmptyObject(v)).forEach(value => {

but I don't know if that's necessary.

The filter as coded will eliminate "undefined" values.

[mkistler@mkistlers-mbp] ~>node > foo = [ undefined ] [ undefined ] > foo.filter(v => v != null) []

Is there another case you are concerned about?

Ah, my mistake. I was thinking about the results when using the more strict !== rather than !=. This looks good as is 👍

dpopp07 · 2019-09-16T16:32:30Z

lib/helper.ts

  return Boolean(obj && obj.value);
 }

 function isFileStream(obj: any): obj is FileStream {
  return obj && isReadable(obj) && obj.path;
 }

+export function isFileParamAttributes(obj: any): obj is FileParamAttributes {
+  return obj && obj.data &&


I don't think this is a big deal but this should maybe be wrapped in a Boolean() cast to ensure the result is either true or false. The way is stands, it could end up returning undefined if obj or obj.data is undefined. That is fine, because it still evaluates to falsy, but it is probably more clear to force the boolean type.

dpopp07 · 2019-09-16T16:33:51Z

lib/helper.ts

  return Boolean(obj && obj.value);
 }

 function isFileStream(obj: any): obj is FileStream {
  return obj && isReadable(obj) && obj.path;
 }

+export function isFileParamAttributes(obj: any): obj is FileParamAttributes {


obj is FileParamAttributes

Cool, I've never seen this! Does this imply a Boolean return value? Did you find any docs/posts about this and if so, could you link me? Looks interesting

dpopp07

Looks good 👍

## [0.3.6](v0.3.5...v0.3.6) (2019-09-16) ### Bug Fixes * Fix handling of array form parameters. ([#43](#43)) ([bad8960](bad8960))

ibm-devx-automation · 2019-09-16T20:12:45Z

🎉 This PR is included in version 0.3.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

mkistler requested a review from dpopp07 September 15, 2019 19:11

fix: Fix handling of array form parameters.

1680c2c

mkistler force-pushed the mdk/array-form-params branch from 9b2bab9 to 1680c2c Compare September 15, 2019 19:19

dpopp07 requested changes Sep 16, 2019

View reviewed changes

dpopp07 approved these changes Sep 16, 2019

View reviewed changes

mkistler merged commit bad8960 into master Sep 16, 2019

mkistler deleted the mdk/array-form-params branch September 16, 2019 20:11

ibm-devx-automation pushed a commit that referenced this pull request Sep 16, 2019

chore(release): 0.3.6 [skip ci]

d9391ef

## [0.3.6](v0.3.5...v0.3.6) (2019-09-16) ### Bug Fixes * Fix handling of array form parameters. ([#43](#43)) ([bad8960](bad8960))

ibm-devx-automation added the released label Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of array form parameters #43

Fix handling of array form parameters #43

mkistler commented Sep 15, 2019

mkistler commented Sep 15, 2019

codecov bot commented Sep 15, 2019 •

edited

Loading

mkistler commented Sep 15, 2019

dpopp07 left a comment

dpopp07 Sep 16, 2019

mkistler Sep 16, 2019

dpopp07 Sep 16, 2019

dpopp07 Sep 16, 2019

dpopp07 Sep 16, 2019

dpopp07 left a comment

ibm-devx-automation commented Sep 16, 2019

	values.filter(v => v != null && !isEmptyObject(v)).forEach(value => {
	values.filter(v => v && !isEmptyObject(v)).forEach(value => {

Fix handling of array form parameters #43

Fix handling of array form parameters #43

Conversation

mkistler commented Sep 15, 2019

mkistler commented Sep 15, 2019

codecov bot commented Sep 15, 2019 • edited Loading

Codecov Report

mkistler commented Sep 15, 2019

dpopp07 left a comment

Choose a reason for hiding this comment

dpopp07 Sep 16, 2019

Choose a reason for hiding this comment

mkistler Sep 16, 2019

Choose a reason for hiding this comment

dpopp07 Sep 16, 2019

Choose a reason for hiding this comment

dpopp07 Sep 16, 2019

Choose a reason for hiding this comment

dpopp07 Sep 16, 2019

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

ibm-devx-automation commented Sep 16, 2019

codecov bot commented Sep 15, 2019 •

edited

Loading