[FOUND-1522 / THRIFT-1976] Allow JS thrift client to use ES6 maps

[alaynarichmond]

The JavaScript thrift client currently uses JS objects to represent Thrift maps: when making requests, the client serializes objects into thrift maps, and when receiving responses, thrift maps are deserialized back into JS objects. There is an issue because the keys of JS objects are always strings. If you try to use an object or other non-primitive value as a key, the .toString() function gets called and the resulting value is used as the key. This means that serialization from JS to thrift does not work as expected when there are complex map keys.

For example:

let key = { "a": "apple" }
let obj = {}
obj[key] = "artichoke"
console.log(obj)


The above code prints out:

{ '[object Object]': 'artichoke' }

Also, if a server sends the client a map with complex keys, deserialization will fail for the same reason.

The fact that the JS client uses objects to represent maps means that the keys of a map must be primitive values, such as strings or numbers. However, the thrift protocol allows any type to be used for the keys of a map, including structs or containers. Thrift implementations in other languages, especially strongly typed languages, support this feature. This problem has been documented in the Apache Thrift JIRA under THRIFT-1976.

One of the suggestions on that issue was to use the ES6 Map type, which allows any JavaScript type to be used for the keys, including objects. I took this suggestion and changed the thrift client so that it can now send and receive maps with structs, containers, etc. as keys. I did not implement the capability to support ES6 maps in the JS thrift server.

For example, the JS client is now able to make requests against the following interface:

struct Bonk {
  1: string message,
  2: i32 type
}

Service ThriftTest {
  map<Bonk,i32> testMapWithStructKey(1: map<Bonk,i32> thing)
  map<list<i32>,i32> testMapWithListKey(1: map<list<i32>,i32> thing)
}

Client example:

const testClient = // JS thrift client generated against the interface above
const Bonk = // imported from the generated thrift types

const mapWithStructKey = new Map();
mapWithStructKey.set(new Bonk({ message: “fruit”,  type: 1 }), 10);
mapWithStructKey.set(new Bonk({ message: “veggie”, type: 2}), 20);
client.testMapWithStructKey(mapWithStructKey).then(
  (response) => {
    console.log(response);
  }
);

const mapWithListKey = new Map();
mapWithListKey.set([1, 2, 3], 10);
mapWithListKey.set([1, 2, 3, 4], 15);
client.testMapWithListKey(mapWithListKey).then(
  (response) => {
    console.log(response);
  }
);

Console output:

Map {
  { message: 'veggie', type: 2 } => 20,
  { message: 'fruit', type: 1 } => 10
}
Map { [ 1, 2, 3, 4 ] => 15, [ 1, 2, 3 ] => 10 }

The changes in this PR are only enabled when the es6 flag is enabled during compilation. Note that the changes are breaking because all thrift maps will be deserialized into ES6 maps instead of objects, which is what they were deserialized into before. When working to get this merged into the official Thrift library, if people prefer a non-breaking change I will work on that. Making this change backwards compatible would involve using objects when the keys are simple values and using ES6 maps when the keys are complex values. However, I think this half and half solution may be more confusing than switching entirely to ES6 maps, which is what I’ve done here.

I have not yet added tests because I’m having trouble getting the test suite to run. I will add those afterwards but would like to get this merged now so we can unblock our other work!

[alaynarichmond]

cc @strava/graphql

[anjiemerchant]

what is happening on line 226? and 231?

i understand that you're following the example on lines 240-256; am wondering if want to use clearer JS syntax (and possibly es6?)

[anjiemerchant]

nice explanation. my only suggestion would be to use consistent examples b/w apple/artichoke and bonk, esp if this is what we'll use to make the PR back to apache

[alaynarichmond]

Merging this to unblock current work. I still need to add tests and will address this feedback at that point! I don't think we have to use this exact PR to contribute back to the Apache repo.