Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

W-16451183 Wr/file flag #14

Merged
merged 14 commits into from
Oct 3, 2024
Merged

W-16451183 Wr/file flag #14

merged 14 commits into from
Oct 3, 2024

Conversation

WillieRuemmele
Copy link
Contributor

What does this PR do?

adds --file flag to request rest command
matches postman collection format e.g. keys are body/header/url/method

What issues does this PR fix or reference?

@W-16451183@

file: Flags.file({
summary: messages.getMessage('flags.file.summary'),
helpValue: 'file',
char: 'f',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pass exists to have oclif validate the file:
https://github.com/oclif/core/blob/f38f8270bafa04cdcb8e6d193b808e099e205864/src/flags.ts#L121

also, we haven't been consistent with this but since this command already has --stream-to-file, what do you think about using --request-file instead?

https://github.com/search?q=org%3Asalesforcecli+%2FFlags%5C.file%5C%28%2F&type=code
https://github.com/salesforcecli/cli/wiki/Design-Guidelines-Flags

const fileOptions = flags.file
? (JSON.parse(fs.readFileSync(flags.file, 'utf8')) as {
body?: string;
header?: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be headers (flag is singular because we expect user to pass it multiple times)

: // otherwise it's a stdin, and we use it directly
flags.body;
} else {
body = JSON.stringify(fileOptions.body);
Copy link
Member

@cristiand391 cristiand391 Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @VivekMChawla @WillieRuemmele

Today --body can be a body string, - to read from stdin or a file path to read the body from.
Since this PR adds support for passing a request as a file I feel we should drop --body from supporting file paths (it's the same feature but we are opening 2 ways to use it).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillieRuemmele - I think that @cristiand391's idea to drop support for file paths in the --body flag makes sense.

messages/rest.md Outdated
# flags.method.summary

HTTP method for the request.

# flags.file.summary

A json file to store values for header/body/method/url - this is the same format as a Postman Collection Format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillieRuemmele @VivekMChawla I couldn't find a way to export a postman request in the collection format.
If it's not possible then we might not need to follow their format?

context:
by not following their format we leave this open to us adding support for parameterized params later:
https://salesforce.quip.com/5t19A9rykzI3#SdNADAvRyDL

Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left a few questions regarding functionality/formats (will ping Vivek) and get back.

@WillieRuemmele WillieRuemmele requested a review from a team as a code owner September 26, 2024 21:57
@cristiand391
Copy link
Member

QA notes:

arg/flags override value in body file

✅ endpoint as ARG
--method
--header

others

✅ HTTP method still defaults to GET if not defined via req body/--method
✅ no endpoint as arg/req body

body format

method is validated
header takes a single header as string or multiple as string[]
header values are validated like in --header
header in postman format (object with key/val) aren't validated:

NOTE: it's missing the key = Accept

{
  "url": "limits",
  "header": [{
    "value": "application/xml"
  }]
}
➜  plugin-api git:(wr/fileFlag) ✗ sf api request rest --file req.json
 ›   Warning: @salesforce/plugin-api is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used instead.
Warning: This command is currently in beta. Any aspect of this command can change without advanced notice. Don't use beta commands in your scripts.
Error (10): Cannot read properties of undefined (reading 'toLowerCase')

✅ raw body mode, can define request body as JSON (gets stringified in payload)
✅ formdata body mode, can upload files
❌ validates body mode

NOTE: it's missing the body.mode = 'raw` key

{
  "url": "sobjects/Account/0018G00000bTTg4QAG",
  "method": "PATCH",
  "body" : {
    "raw": {
      "name": "Mr Doe"
    }
  }
}
➜  plugin-api git:(wr/fileFlag) ✗ sf api request rest --file req.json
 ›   Warning: @salesforce/plugin-api is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used instead.
Warning: This command is currently in beta. Any aspect of this command can change without advanced notice. Don't use beta commands in your scripts.
Error (10): Cannot read properties of undefined (reading 'map')

we should either default to raw if it's not defined or validate it's present, example in help text with body doesn't specify body mode:

    Here's a simple example of a JSON file that contains values for the request URL, method, and body:

    {
    "url": "sobjects/Account/<Account ID>",
    "method": "PATCH",
    "body" : {"BillingCity": "Boise"}
    }

    See more examples in the plugin-api test directory, including JSON files that use "formdata" to define collections:
    https://github.com/salesforcecli/plugin-api/tree/main/test/test-files/data-project.

@cristiand391
Copy link
Member

QA update:

❌ validates body mode

✅ fixed:

➜  plugin-api git:(wr/fileFlag) ✗ sf api request rest --file req.json
 ›   Warning: @salesforce/plugin-api is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used instead.
Warning: This command is currently in beta. Any aspect of this command can change without advanced notice. Don't use beta commands in your scripts.
Error (SfError): No 'mode' found in 'body' entry
                                                                                                                                                                        
Try this:
                                                                                                                                                                        
add "mode":"raw" | "formdata" to your body
                                                                                                                                                                        
➜  plugin-api git:(wr/fileFlag) ✗ echo $?
1

❌ header in postman format (object with key/val) aren't validated:

✅ fixed

➜  plugin-api git:(wr/fileFlag) ✗ sf api request rest --file req.json
 ›   Warning: @salesforce/plugin-api is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used instead.
Warning: This command is currently in beta. Any aspect of this command can change without advanced notice. Don't use beta commands in your scripts.
Error (SfError): Failed to validate header: missing key: undefined or value: application/xml

➜  plugin-api git:(wr/fileFlag) ✗ echo $?
1
➜  plugin-api git:(wr/fileFlag) ✗

@cristiand391 cristiand391 merged commit aef0d4b into main Oct 3, 2024
13 checks passed
@cristiand391 cristiand391 deleted the wr/fileFlag branch October 3, 2024 18:36
@iowillhoit iowillhoit changed the title Wr/file flag W-16451183 Wr/file flag Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants