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

Add response metadata to slack response #772

Merged

Conversation

alyosha
Copy link
Contributor

@alyosha alyosha commented Aug 9, 2020

Summary of Changes

The Web API response represented by the SlackResponse struct can contain an additional JSON field "response_metadata" in such cases as block validation failure, etc. It would be helpful to include the field in the struct so that these extra descriptors can be returned in existing models like ViewResponse when something goes wrong.

Example

Below are two examples of what the ViewResponse looks like when dumped before/after the addition.

Before

(*slack.ViewResponse)(0xc000419320)({
   SlackResponse: (slack.SlackResponse) {
       Ok: (bool) false,
       Error: (string) (len=17) "invalid_arguments"
    },
    ...
}

After

(*slack.ViewResponse)(0xc000778500)({
    SlackResponse: (slack.SlackResponse) {
        Ok: (bool) false,                                                                                                                                            
        Error: (string) (len=17) "invalid_arguments",                                                                                                                
        ResponseMetadata: (slack.ResponseMetadata) {                                                                                                                 
           Cursor: (string) "",                                                                                                                                        
           Messages: ([]string) (len=3 cap=4) {                                                                                                                        
              (string) (len=64) "[ERROR] failed to match all allowed schemas [json-pointer:/view]",                                                                      
              (string) (len=70) "[ERROR] failed to match all allowed schemas [json-pointer:/view/title]",                                                                
              (string) (len=71) "[ERROR] must be less than 25 characters [json-pointer:/view/title/text]"                                                                
           },                                                                                                                                                          
           Warnings: ([]string) (len=1 cap=4) {                                                                                                                        
             (string) (len=15) "missing_charset"                                                                                                                        
           }                                                                                                                                                           
        } 
    },
    ...
}

Copy link
Contributor

@ollieparsley ollieparsley left a comment

Choose a reason for hiding this comment

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

Hi @alyosha. Thank you for this. Is godebug actually used? I can't see any references to it other than in the modules and vendor list. If it isn't used (could be that I'm missing it), could you remove it please. Then I can get this merged.

@ollieparsley ollieparsley added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Aug 15, 2020
Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

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

In addition to @ollieparsley's comments, I commented a bit :)

misc.go Outdated
Error string `json:"error"`
Ok bool `json:"ok"`
Error string `json:"error"`
ResponseMetadata map[string]interface{} `json:"response_metadata"`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can use struct { Message []string } as the type of response_metadata instead of map[string]interface{}.

e.g. official Java Implementation: https://github.com/slackapi/java-slack-sdk/blob/main/slack-api-model/src/main/java/com/slack/api/model/ResponseMetadata.java

Copy link
Contributor Author

@alyosha alyosha Aug 18, 2020

Choose a reason for hiding this comment

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

hey @kanata2 thanks for your feedback! I would be open to using a struct instead of a map here, will update

@alyosha
Copy link
Contributor Author

alyosha commented Aug 18, 2020

hey @ollieparsley it's used in test but this is purely a personal preference (just wanted to check all fields match in the expected/received struct, where before only a few were being checked)

I will drop in lieu of reflect though, just to avoid introducing new deps 👍

@alyosha alyosha force-pushed the add-response-metadata-to-slack-response branch from 6d8aacd to b0eeb26 Compare September 12, 2020 07:51
@alyosha
Copy link
Contributor Author

alyosha commented Sep 19, 2020

@ollieparsley @kanata2

Thanks again for the reviews and sorry for the delay in updating 👋

I made the move to use a struct rather than map as @kanata2 suggested, but I noticed there are already multiple ResponseMetadata types floating around in the library. I decided to just add the new metadata fields (messages/warnings) to one of the existing structs, please let me know if you have any concerns here.

I confirmed the behavior using a personal project and updated the PR description with an example of what the metadata looks like following these changes

@stevekuznetsov
Copy link

Fixes #805

Would love to see this merge, otherwise errors are not debuggable :)

@kanata2 kanata2 merged commit a62f562 into slack-go:master Sep 29, 2020
@alyosha alyosha deleted the add-response-metadata-to-slack-response branch September 30, 2020 02:40
@kanata2 kanata2 added this to the v0.7.0 milestone Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback given When a review has been conducted and awaiting the response from the comitter(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants