trayio / falafel Goto Github PK

We now have a help parameter attached to connectors. As such, we can now start to build up special content and helpful links for all connectors.

Suggestion: add a help.md file on the connector level. The contents of this will get set to the help property in the connectors.json.

Returning blank strings is bad. Consider erroring, or responding with a blank object instead.

E.g. passing an object to a string, array to an object, etc. Right now a lot of customers pass data in the incorrect format to different variables, and we should do what we can to let them know in the logs of their mistake.

Related to this other idea #47

For items of type array, in generateSchemaFromJs, support for array of schemas needed.
Atm, only objects supported.

 Luka

graegraegre

 Problem overview

htshtht

When you have a nested object in the data property such as

{
    parameters: [
        "abc",
        "def"
    ]
}

is ending up like this

{
    parameters: {
        "0": "abc",
        "1": "def"
    }
}

Different systems have different date formats. Example:

• Salesforce: 2016-02-18T21:54:33.000+0000
• Zendesk: 2016-02-18T21:54:33Z

Zendesk actually rejects dates that are in the Salesforce format - causing an error in the workflow. The data needs to be formatted in a JavaScript step to be able to pass a date to Zendesk correctly.

Proposal:

• Add a "date" format to the connectors json
• Add a "date_format" key to schema properties to indicate the format a date should have
  The connector should automatically try and convert the date to the desired format

Why?

• Simpler and more reliable for users!

At least it should not print the input, because it contains sensitive information and there would be no easy way to mask it in CS

{
    "code": "ECONNREFUSED",
    "message": "connect ECONNREFUSED 127.0.0.1:80",
    "payload": {
      "code": "ECONNREFUSED",
      "errno": "ECONNREFUSED",
      "syscall": "connect",
      "address": "127.0.0.1",
      "port": 80,
      "input": {
        "access_token": "<removed>",
        "contact_id": "watha"
      }
    }
}

Invalid connectors = bad. We should run programmatic validation wherever possible to minimise errors and time wasted.

Current situation

Currently falafel supports file uploads and downloads, like so:

"Download" attachment

Downloads an attachment from a third party service, e.g. Salesforce, and saves it to S3. The flow is:

Get file contents > Store in memory (buffer) > Upload to S3

"Upload" attachment

Uploads an attachment to a third party service, e.g. Asana. The flow is:

Download file from S3 URL > Store in memory (buffer) > Upload to service

Key issue

Files are stored in memory. For an 8MB zip file, a 1.5GB lambda function ran out of memory while executing. No idea why, but it looks like an exponential effect.

New approach

Use the filesystem to store the files temporarily in the connector Lambda filesystem, rather than memory. This approach has already been tested and working on production with the Salesforce connector - it's at least an order of magnitude more efficient, and unlikely to be exponential.

One of the key advantages here is that we can make use of Node.js streams, which allow the data to be passed along as it is output & read, rather than having to store everything in memory first and then send it all at once.

Downloads

The flow will now be:

Get file contents > Output directly to temp file > Upload file to S3

Uploads

The flow will now be:

Download file from S3 > Output to temp file > Upload file to service

Implementation

Add a fileHandler parameter to models

Both of the above flows require a temporary file to be created and read from on the local filesystem.

Lambda allows the user to write to the local filesystem in the /tmp folder, and falafel models already allow the outputting to a file (for downloads) with the following options:

options: {
  output: '{{temp_file}}'
}

Similarly for uploads, the S3 file needs to be downloaded to a temporary file on the filesystem, so a temp_file will also be needed.

Proposal: add a new fileHandler: true option to models, to be specified on any download or upload operations.

The above will cause an additional temp_file input parameter to be added to the incoming message from the Cluster Service, in the format /tmp/guid.

The connector can then use this parameter to download & upload from, without having to go into the detail of adding this parameter itself.

File upload API change

Backwards compatibility is important here.

Upload

falafel.files.upload - currently takes the following input:

{
  name: 'my-file-name.pdf',
  contents: Buffer(....)
}

New versions should take:

{
  name: 'my-file-name.pdf',
  file: '{{temp_file}}'
}

Download

falafel.files.download - the input won't change here, but the output will. It currently outputs:

{
  contents: Buffer(...),
  name: 'my-file-name.pdf',
  mime_type: 'application/pdf',
  expires: 124513421321
}

New versions should output:

{
  file: '{{temp_file}}',
  name: 'my-file-name.pdf',
  mime_type: 'application/pdf',
  expires: 124513421321
}

Every API is different about how it throws back errors. It's quite important that we show a useful message to the user, rather than "Bad request" or "Invalid status code".

We need a JS function on the global level which automatically tries to pull out errors from the body on failure.

This line https://github.com/trayio/falafel/blob/master/lib/bindConnectors/getMissingParams.js#L22 causes any connector message without a schema.js file to error - because it's trying to get property globals of undefined.

Probably the simplest way to do this is to add a order integer value on the schema which determines the priority the operation should have. Lowest goes first.

Let's say someone passes a string to a connector, but it should be a number. However, it's a numeric string. The third party API might error unless the type is a pure number.

Wouldn't it be smart if we automatically typecasted the parameter to a number? It'd boost the reliability of connectors significantly.

Else, it get's to bindMessage and fails.

Guard the message format to never return invalid json structure (for example a "body" that is not a json object), and return an error message in such cases. "Invalid API response, expected json object"

Falafel is returning the whole connector/operation input object when there is an error of any sort, which often includes sensitive information such as access tokens. The returned payload gets stored in the step output by the cluster service which is not obfuscated in any way.

Need to remove the input object from the returned payload on errors

This message doesn't get shown when a required parameter is missing on global:

A user should just have to input their bugsnag token and that's it.

Useful for debugging etc.

Sniper intercepts webhook triggers and maps them into a format that goes to the connector.

Currently if you want to test trigger connectors end-to-end offline then you can't with ngrok. It'd be great if you could.

Seems like this basically needs a proxy system in dev mode to route data to the connector in the same way sniper does.

Atm, get missing params only works on top level.
Refactor so that it works for deeply nested requireds and oneOf.

Ditch requireindex.

Currently it order by name which is not as good:

404 = not found
403 = forbidden

Etc. Right now we mostly just say "Invalid status code". Could be smarter which would be better UX.

For a lot of error types, we respond with "Invalid status code", which isn't at all helpful for the user. We already respond with "Not found" for 404, for example - what else could we make simpler here?

Definitely handle:

• 429 - rate limits

See http://www.restapitutorial.com/httpstatuscodes.html

These are actually handled in threadneedle (rather than falafel) here https://github.com/trayio/threadneedle/blob/master/lib/addMethod/validateExpects.js#L72

Note that you'll need unit tests for anything you add too https://github.com/trayio/threadneedle/blob/master/tests/validateExpects_test.js#L38

First step - I'd recommend putting together a list of status codes we should handle, and the error message for each. (Note that for many of the 5xx errors, we might want to use the same error message)

markdown works

Every single

time

See this error I get when trying to deploy a connector to staging:

It says there's an error, but doesn't provide any context as to where it's coming from. Would be useful if it did!

Probably should only do this if the schema.js does exist. Would save a precious few seconds going to postman to copy the contents.

Or a step further - always dynamically generate it? Room for thought.

Here's the schema for the fields key:

And here's the connectors.json:

This is the delivery mode, used by CS

Internally what it will do is:

• Run the main method
• Allow you to format the output for DDL
• Maybe allow the flexibility to run extra methods too if needed

Would be good to think about providing a helper for the DDL schema data.