ref: Use Rate Limits #10
Conversation
mgaeta
force-pushed
the
marcos/ref/rate-limits
branch
from
f3cfe03 to
fd8c419
Compare
mgaeta
force-pushed
the
marcos/test/server
branch
from
8c42f12 to
2371b88
Compare
mgaeta
force-pushed
the
marcos/ref/rate-limits
branch
from
fd8c419 to
869ec4f
Compare
| @@ -12,3 +12,14 @@ func annotationsForUserResourceType() annotations.Annotations { | |||
| annos.Update(&v2.SkipEntitlementsAndGrants{}) | |||
| return annos | |||
| } | |||
|
|
|||
| func WithRateLimitAnnotations( | |||
There was a problem hiding this comment.
A function like this one should probably live in baton-sdk at some point.
| zap.String("id", group.Id), | ||
| zap.String("name", group.Name), | ||
| ) | ||
| bag.Push( |
There was a problem hiding this comment.
I think this will exceed the max page token size of 4096 characters if the customer has a lot of groups.
There was a problem hiding this comment.
Should I bring down the page size when getting the list of groups? Is there a way to make sure that there is only one page worth of groups in the bag at a time?
There was a problem hiding this comment.
Hard to say. Also I'm pretty sure it's possible to have a user that isn't a member of any group.
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughThe recent updates significantly enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
Client->>API: Request Groups
API-->>Client: Return Group Data with Rate Limit Info
Client->>API: Request Group Members
API-->>Client: Return Members Data with Rate Limit Info
Client->>API: Add User to Group
API-->>Client: Confirm User Added, Return Rate Limit Info
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (4)
test/fixtures/blank.jsonis excluded by none and included by nonetest/fixtures/search0.jsonis excluded by none and included by nonetest/fixtures/search1.jsonis excluded by none and included by nonetest/testhelpers.gois excluded by none and included by none
Files selected for processing (8)
- pkg/connector/client/confluence.go (5 hunks)
- pkg/connector/client/models.go (2 hunks)
- pkg/connector/client/path.go (1 hunks)
- pkg/connector/connector.go (1 hunks)
- pkg/connector/group.go (8 hunks)
- pkg/connector/helpers.go (1 hunks)
- pkg/connector/user.go (4 hunks)
- pkg/connector/user_test.go (2 hunks)
Additional comments not posted (39)
pkg/connector/helpers.go (1)
16-25: LGTM! Consider future improvements.The function
WithRateLimitAnnotationsis well-implemented and correctly handles dynamic rate limit annotations.Future improvement suggestion: Consider adding error handling or validation for the provided annotations to ensure they meet expected criteria.
pkg/connector/user_test.go (2)
35-35: LGTM! Consider future improvements.The change to initialize
pTokenwithSize: 2improves the test by specifying the pagination size.Future improvement suggestion: Consider adding more test cases to cover different pagination sizes and edge cases to ensure robustness.
59-59: LGTM! Consider future improvements.The change to expect the length of
resourcesto be 3 reflects the updated pagination logic.Future improvement suggestion: Consider adding assertions to verify the contents of
resourcesto ensure they meet expected criteria.pkg/connector/client/models.go (7)
3-6: LGTM!The type
ConfluenceLinkis well-defined with appropriate JSON tags.
9-12: LGTM!The JSON tags for the
ConfluenceUsertype are appropriate and enhance API flexibility.
16-21: LGTM!The JSON tags and the new
Linksfield for theconfluenceUserListtype enhance the structure of user list responses.
23-28: LGTM!The type
ConfluenceSearchis well-defined with appropriate JSON tags.
30-35: LGTM!The type
ConfluenceSearchListis well-defined with appropriate JSON tags.
45-50: LGTM!The JSON tags and the new
Linksfield for theconfluenceGroupListtype enhance the structure of group list responses.
52-53: LGTM!The type
AddUserToGroupRequestBodyis well-defined with an appropriate JSON tag.pkg/connector/connector.go (1)
16-16: LGTM! The new constant improves flexibility.The addition of
accountTypeAppenhances the flexibility of the account management logic by allowing differentiation between user accounts and bot accounts.pkg/connector/client/path.go (5)
9-22: LGTM! The new constants improve readability.The introduction of constants for URL paths centralizes the paths and enhances code readability.
26-49: LGTM! Consider supporting additional types.The
withQueryParametersfunction correctly handles different types of query parameters and constructs the query string. Consider adding support for additional types if necessary.
52-64: LGTM! The pagination parameters are correctly added.The
withLimitAndOffsetfunction correctly addsstartandlimitparameters to the URL for pagination.
66-84: LGTM! The function handles pagination parameters correctly.The
WithPaginationCursorfunction correctly handles pagination parameters using Confluence Cloud's REST API v2 pagination scheme.
86-102: LGTM! The function correctly parses the URL and applies options.The
parsefunction correctly parses the URL and applies the provided options.pkg/connector/user.go (5)
Line range hint
10-48:
LGTM! The function constructs the user resource correctly.The
userResourcefunction correctly constructs the user resource with profile and trait options, and handles errors appropriately.
51-81: LGTM! The function handles pagination token parsing correctly.The
parsePageTokenfunction correctly parses the pagination token and returns the pagination bag, page token, and page size.
Line range hint
83-134:
LGTM! The function handles pagination and rate limits correctly.The
Listfunction correctly handles pagination, rate limit annotations, and constructs the user resources.
137-146: LGTM! The function correctly returns nil values.The
Entitlementsfunction correctly returns nil values as it is not yet implemented.
150-159: LGTM! The function correctly returns nil values.The
Grantsfunction correctly returns nil values as it is not yet implemented.pkg/connector/group.go (6)
Line range hint
38-50:
LGTM! Improved variable naming for clarity.The renaming of
resourcetonewGroupResourceenhances readability and clarity.
Line range hint
53-96:
LGTM! Enhanced method utility with rate limit annotations.The inclusion of
outputAnnotationsin the return type provides additional context about rate limits, improving the method's utility.
Line range hint
99-125:
LGTM! Enhanced method utility with rate limit annotations.The inclusion of
outputAnnotationsin the return type provides additional context about rate limits, improving the method's utility.
Line range hint
126-181:
LGTM! Enhanced method utility with rate limit annotations.The inclusion of
outputAnnotationsin the return type provides additional context about rate limits, improving the method's utility.
183-195: LGTM! New method for adding users to groups.The new
Grantmethod encapsulates the logic for adding users to groups and provides rate limit information, enhancing the API's usability.
197-207: LGTM! New method for removing users from groups.The new
Revokemethod encapsulates the logic for removing users from groups and provides rate limit information, enhancing the API's usability.pkg/connector/client/confluence.go (12)
103-132: LGTM! New method for retrieving groups from Confluence.The new
GetGroupsmethod encapsulates the logic for retrieving groups from Confluence and provides rate limit information, enhancing the API's usability.
135-165: LGTM! New method for retrieving group members from Confluence.The new
GetGroupMembersmethod encapsulates the logic for retrieving group members from Confluence and provides rate limit information, enhancing the API's usability.
168-198: LGTM! New method for adding a user to a group.The new
AddUserToGroupmethod encapsulates the logic for adding a user to a group and provides rate limit information, enhancing the API's usability.
200-221: LGTM! New method for removing a user from a group.The new
RemoveUserFromGroupmethod encapsulates the logic for removing a user from a group and provides rate limit information, enhancing the API's usability.
223-229: LGTM! Refactored to use centralized request handling.The refactoring to use
makeRequestcentralizes the request handling logic, improving maintainability and consistency in rate limit handling.
231-238: LGTM! Refactored to use centralized request handling.The refactoring to use
makeRequestcentralizes the request handling logic, improving maintainability and consistency in rate limit handling.
240-245: LGTM! Refactored to use centralized request handling.The refactoring to use
makeRequestcentralizes the request handling logic, improving maintainability and consistency in rate limit handling.
248-292: LGTM! New method for centralized request handling.The new
makeRequestmethod encapsulates the logic for making HTTP requests and handling rate limits, improving maintainability and consistency in the codebase.
296-303: LGTM! Improved URL construction.The refactoring improves code readability and reduces the risk of errors in URL construction.
306-326: LGTM! Improved URL construction.The refactoring improves code readability and reduces the risk of errors in URL construction.
348-361: LGTM! New method for extracting rate limit information.The new
WithConfluenceRatelimitDatamethod improves the handling of rate limit data, ensuring that the client can adapt its behavior based on the API's feedback.
363-381: LGTM! New method for checking rate limit conditions.The new
isRatelimitedmethod encapsulates the logic for determining if a response is rate-limited, improving the client's ability to handle rate limit conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- pkg/connector/user.go (4 hunks)
Additional comments not posted (5)
pkg/connector/user.go (5)
Line range hint
38-48:
LGTM!The variable name change from
resourcetonewUserResourceimproves clarity.
140-149: No significant changes.The function
Entitlementshas no significant changes and returns nil values.
153-162: No significant changes.The function
Grantshas no significant changes and returns nil values.
Line range hint
164-168:
No significant changes.The function
userBuilderhas no significant changes and initializes auserResourceType.
Line range hint
86-137:
LGTM! But verify the function usage in the codebase.The refactoring improves readability and maintainability. Ensure that all function calls to
Listare updated to match the new implementation.
pkg/connector/user.go
Outdated
| // parsePageToken given a marshalled pageToken as a string, return the pageToken | ||
| // bag and the current page number. | ||
| func parsePageToken( | ||
| pToken *pagination.Token, | ||
| resourceID *v2.ResourceId, | ||
| ) ( | ||
| *pagination.Bag, | ||
| string, | ||
| int, | ||
| error, | ||
| ) { | ||
| b := &pagination.Bag{} | ||
| err := b.Unmarshal(pToken.Token) | ||
| if err != nil { | ||
| return nil, "0", 0, err | ||
| } | ||
|
|
||
| if b.Current() == nil { | ||
| b.Push(pagination.PageState{ | ||
| ResourceTypeID: resourceID.ResourceType, | ||
| ResourceID: resourceID.Resource, | ||
| }) | ||
| } | ||
|
|
||
| page := b.PageToken() | ||
| size := pToken.Size | ||
| if size == 0 { | ||
| size = ResourcesPageSize | ||
| } | ||
| if page == "" { | ||
| page = "0" | ||
| } | ||
| return b, page, size, nil | ||
| } |
There was a problem hiding this comment.
Well-structured function! Consider adding unit tests.
The parsePageToken function modularizes the pagination logic effectively. Ensure that unit tests are added to verify its functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
There was a problem hiding this comment.
@coderabbitai please generate them
There was a problem hiding this comment.
@jirwin, here are the unit tests for the parsePageToken function:
package connector
import (
"testing"
"github.com/conductorone/baton-sdk/pkg/pagination"
v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2"
"github.com/stretchr/testify/assert"
)
func TestParsePageToken(t *testing.T) {
tests := []struct {
name string
token *pagination.Token
resourceID *v2.ResourceId
expectedPage string
expectedSize int
expectError bool
}{
{
name: "Valid token with size",
token: &pagination.Token{
Token: "validToken",
Size: 5,
},
resourceID: &v2.ResourceId{
ResourceType: "testType",
Resource: "testResource",
},
expectedPage: "validToken",
expectedSize: 5,
expectError: false,
},
{
name: "Valid token without size",
token: &pagination.Token{
Token: "validToken",
},
resourceID: &v2.ResourceId{
ResourceType: "testType",
Resource: "testResource",
},
expectedPage: "validToken",
expectedSize: ResourcesPageSize,
expectError: false,
},
{
name: "Invalid token",
token: &pagination.Token{
Token: "invalidToken",
},
resourceID: &v2.ResourceId{
ResourceType: "testType",
Resource: "testResource",
},
expectedPage: "",
expectedSize: 0,
expectError: true,
},
{
name: "Token with no current page",
token: &pagination.Token{
Token: "",
},
resourceID: &v2.ResourceId{
ResourceType: "testType",
Resource: "testResource",
},
expectedPage: "0",
expectedSize: ResourcesPageSize,
expectError: false,
},
{
name: "Token with existing current page",
token: &pagination.Token{
Token: "existingPage",
},
resourceID: &v2.ResourceId{
ResourceType: "testType",
Resource: "testResource",
},
expectedPage: "existingPage",
expectedSize: ResourcesPageSize,
expectError: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
bag, page, size, err := parsePageToken(tt.token, tt.resourceID)
if tt.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.NotNil(t, bag)
assert.Equal(t, tt.expectedPage, page)
assert.Equal(t, tt.expectedSize, size)
}
})
}
}These tests cover various scenarios to ensure the parsePageToken function works as expected.
Learnings added
Learnt from: jirwin
PR: ConductorOne/baton-confluence#10
File: pkg/connector/user.go:51-84
Timestamp: 2024-07-30T20:10:33.633Z
Learning: Unit tests for the `parsePageToken` function in `pkg/connector/user.go` should cover scenarios including valid tokens with and without specified sizes, invalid tokens, tokens with no current page, and tokens with an existing current page.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| ) { | ||
| getUsersUrl, err := c.parse( | ||
| SearchUrlPath, | ||
| withLimitAndOffset(pageToken, pageSize), |
There was a problem hiding this comment.
This works once you add withQueryParameters(map[string]interface{}{"cql": "type=user"}),
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- pkg/connector/client/confluence.go (5 hunks)
Additional comments not posted (19)
pkg/connector/client/confluence.go (19)
Line range hint
61-75:
LGTM! Ensure error handling is robust.The function correctly initializes the ConfluenceClient and handles errors appropriately.
80-91: LGTM! Ensure error handling is robust.The function correctly verifies the current user and handles errors appropriately.
99-101: LGTM!The function correctly checks if there is another page of results.
103-132: LGTM! Ensure robust error handling.The function correctly retrieves groups, handles pagination, and manages rate limits. Ensure the error handling is robust.
135-165: LGTM! Ensure robust error handling.The function correctly retrieves group members, handles pagination, and manages rate limits. Ensure the error handling is robust.
168-198: LGTM! Ensure robust error handling.The function correctly adds a user to a group and manages rate limits. Ensure the error handling is robust.
200-221: LGTM! Ensure robust error handling.The function correctly removes a user from a group and manages rate limits. Ensure the error handling is robust.
223-229: LGTM! Ensure robust error handling.The function correctly makes a GET request and manages rate limits. Ensure the error handling is robust.
231-238: LGTM! Ensure robust error handling.The function correctly makes a POST request and manages rate limits. Ensure the error handling is robust.
240-245: LGTM! Ensure robust error handling.The function correctly makes a DELETE request and manages rate limits. Ensure the error handling is robust.
248-292: LGTM! Ensure robust error handling.The function correctly makes an HTTP request, manages rate limits, and handles errors appropriately.
296-303: LGTM!The function correctly generates a non-paginated URL.
306-326: LGTM!The function correctly generates a paginated URL.
Line range hint
328-335:
LGTM!The function correctly increments the pagination token.
Line range hint
337-345:
LGTM!The function correctly converts a string to an integer.
348-361: LGTM! Ensure robust error handling.The function correctly extracts rate limit data from the response and handles errors appropriately.
363-381: LGTM!The function correctly checks if the request is rate limited.
383-428: Ensure error handling is robust and verify past comments.The function correctly retrieves users using a search query, handles pagination, and manages rate limits. Ensure the error handling is robust.
Additionally, verify the past comment by
ggreerregarding the addition of query parameters.
11-13: Ensure new imports are necessary.The imports for "slices", "strconv", and "strings" should be verified to ensure they are used correctly and are necessary.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (5)
test/fixtures/search0.jsonis excluded by none and included by nonetest/fixtures/search1.jsonis excluded by none and included by nonetest/fixtures/users0.jsonis excluded by none and included by nonetest/fixtures/users1.jsonis excluded by none and included by nonetest/testhelpers.gois excluded by none and included by none
Files selected for processing (6)
- pkg/connector/client/confluence.go (5 hunks)
- pkg/connector/client/models.go (2 hunks)
- pkg/connector/group.go (8 hunks)
- pkg/connector/helpers.go (2 hunks)
- pkg/connector/user.go (4 hunks)
- pkg/connector/user_test.go (3 hunks)
Additional context used
Learnings (2)
pkg/connector/user_test.go (1)
Learnt from: jirwin PR: ConductorOne/baton-confluence#10 File: pkg/connector/user.go:51-84 Timestamp: 2024-07-30T20:10:33.777Z Learning: Unit tests for the `parsePageToken` function in `pkg/connector/user.go` should cover scenarios including valid tokens with and without specified sizes, invalid tokens, tokens with no current page, and tokens with an existing current page.pkg/connector/user.go (1)
Learnt from: jirwin PR: ConductorOne/baton-confluence#10 File: pkg/connector/user.go:51-84 Timestamp: 2024-07-30T20:10:33.777Z Learning: Unit tests for the `parsePageToken` function in `pkg/connector/user.go` should cover scenarios including valid tokens with and without specified sizes, invalid tokens, tokens with no current page, and tokens with an existing current page.
Additional comments not posted (28)
pkg/connector/helpers.go (2)
21-30: LGTM!The
WithRateLimitAnnotationsfunction correctly constructs anannotations.Annotationsobject by appending each providedRateLimitDescriptionannotation.
32-44: LGTM!The
shouldIncludeUserfunction correctly filters users based on their account type and operational status, with appropriate logging.pkg/connector/user_test.go (2)
35-35: LGTM!The initialization of the
pTokenvariable with aSizefield set to 2 enhances the specificity of the pagination mechanism.
59-59: LGTM!The update to the expected length of the
resourcesslice from 2 to 3 reflects the revised expectation for the number of resources returned.pkg/connector/client/models.go (7)
3-6: LGTM!The
ConfluenceLinktype is correctly defined with appropriate JSON tags forBaseandNext.
9-14: LGTM!The JSON tags for the
ConfluenceUsertype are correctly added, and theomitemptytag for the
22-27: LGTM!The JSON tags and the new
Linksfield for theconfluenceUserListtype are correctly added, enhancing the structure for handling related links in user list responses.
29-34: LGTM!The
ConfluenceSearchtype is correctly defined with appropriate JSON tags for search results.
36-41: LGTM!The
ConfluenceSearchListtype is correctly defined with appropriate JSON tags to include pagination details alongside a list of search results.
51-56: LGTM!The JSON tags and the new
Linksfield for theconfluenceGroupListtype are correctly added, enhancing the structure for handling related links in group list responses.
58-59: LGTM!The
AddUserToGroupRequestBodytype is correctly defined with an appropriate JSON tag forAccountId.pkg/connector/user.go (5)
Line range hint
38-48: LGTM! The variable renaming improves clarity.The renaming of the return variable from
resourcetonewUserResourceenhances readability without affecting functionality.
139-148: LGTM! The changes improve readability and provide additional context.The changes to the
Entitlementsmethod's signature format and return type enhance readability and provide additional contextual information throughoutputAnnotations.
152-161: LGTM! The changes improve readability and provide additional context.The changes to the
Grantsmethod's signature format and return type enhance readability and provide additional contextual information throughoutputAnnotations.
Line range hint
86-136: Refactored method looks good! Ensure comprehensive testing.The refactoring of the
Listmethod improves separation of concerns and logging. Ensure that the method handles errors and edge cases correctly.
51-84: Well-structured function! Ensure unit tests are added.The
parsePageTokenfunction modularizes the pagination logic effectively. Ensure that unit tests are added to verify its functionality.pkg/connector/group.go (5)
Line range hint
38-48: LGTM! The variable renaming improves clarity.The renaming of the return variable from
resourcetonewGroupResourceenhances readability without affecting functionality.
97-106: LGTM! The changes improve readability and provide additional context.The changes to the
Entitlementsmethod's signature format and return type enhance readability and provide additional contextual information throughoutputAnnotations.
Line range hint
124-177: LGTM! The changes improve readability and provide additional context.The changes to the
Grantsmethod's signature format and return type enhance readability and provide additional contextual information throughoutputAnnotations.
179-191: New method looks good! Ensure comprehensive testing.The
Grantmethod encapsulates the logic for adding users to groups and handles rate limit data. Ensure that the method is thoroughly tested.Verification successful
The
Grantmethod is indirectly tested through theGrantsmethod. Ensure comprehensive testing.The
Grantmethod is covered by existing tests for theGrantsmethod in thepkg/connector/group_test.gofile. Ensure that these tests are comprehensive and cover all edge cases for theGrantmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the functionality of the `Grant` method. # Test: Search for the unit test function. Expect: Unit tests for `Grant` method. rg --type go 'func TestGroupResourceType_Grant' --context 5Length of output: 61
Script:
#!/bin/bash # Description: Verify the functionality of the `Grant` method by searching for any references to it in test files. # Search for any references to the `Grant` method in Go test files. rg --type go 'Grant' -g '*_test.go' --context 5Length of output: 884
Script:
#!/bin/bash # Description: Verify the relationship between the `Grants` and `Grant` methods by searching for the definition of the `Grants` method in the codebase. # Search for the definition of the `Grants` method in Go files. rg --type go 'func Grants' --context 5Length of output: 40
Script:
#!/bin/bash # Description: Verify the presence of methods related to `Grant` in the `pkg/connector/group.go` file. # Search for all method definitions in the `pkg/connector/group.go` file. rg --type go '^func ' pkg/connector/group.go --context 5Length of output: 1833
Script:
#!/bin/bash # Description: Verify the presence of tests for the `Grants` method in the `pkg/connector/group_test.go` file. # Search for references to the `Grants` method in the `pkg/connector/group_test.go` file. rg --type go 'Grants' pkg/connector/group_test.go --context 5Length of output: 424
Line range hint
51-94: Refactored method looks good! Ensure comprehensive testing.The refactoring of the
Listmethod improves pagination and error handling. Ensure that the method handles errors and edge cases correctly.pkg/connector/client/confluence.go (7)
Line range hint
61-75: LGTM! The initialization changes improve clarity.The changes to the initialization of the
ConfluenceClientenhance readability without affecting functionality.
80-91: LGTM! The changes improve error handling.The changes to the
Verifymethod enhance error handling and ensure that the current user is correctly identified.
103-132: New method looks good! Ensure comprehensive testing.The
GetGroupsmethod encapsulates the logic for retrieving groups and handles rate limit data. Ensure that the method is thoroughly tested.
174-204: New method looks good! Ensure comprehensive testing.The
AddUserToGroupmethod encapsulates the logic for adding users to groups and handles rate limit data. Ensure that the method is thoroughly tested.
237-244: New method looks good! Ensure comprehensive testing.The
postmethod encapsulates the logic for making HTTP POST requests and handles rate limit data. Ensure that the method is thoroughly tested.#!/bin/bash # Description: Verify the functionality of the `post` method. # Test: Search for the unit test function. Expect: Unit tests for `post` method. rg --type go 'func TestConfluence --- `229-235`: **Refactored method looks good! Ensure comprehensive testing.** The refactoring of the `get` method improves the handling of HTTP GET requests and rate limit data. Ensure that the method is thoroughly tested. --- `206-227`: **New method looks good! Ensure comprehensive testing.** The `RemoveUserFromGroup` method encapsulates the logic for removing users from groups and handles rate limit data. Ensure that the method is thoroughly tested. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| func (o *groupResourceType) Revoke( | ||
| ctx context.Context, | ||
| grant *v2.Grant, | ||
| ) (annotations.Annotations, error) { | ||
| ratelimitData, err := o.client.RemoveUserFromGroup( | ||
| ctx, | ||
| grant.Entitlement.Resource.Id.Resource, | ||
| grant.Principal.Id.Resource, | ||
| ) | ||
| outputAnnotations := WithRateLimitAnnotations(ratelimitData) | ||
| return outputAnnotations, err |
There was a problem hiding this comment.
Tip
Codebase Verification
Comprehensive testing needed for Revoke method
The Revoke method in groupResourceType does not appear to have any direct unit tests. To ensure its functionality and reliability, it is crucial to add comprehensive tests for this method.
- File:
pkg/connector/group.go - Lines: 193-203
Analysis chain
New method looks good! Ensure comprehensive testing.
The Revoke method encapsulates the logic for removing users from groups and handles rate limit data. Ensure that the method is thoroughly tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the functionality of the `Revoke` method.
# Test: Search for the unit test function. Expect: Unit tests for `Revoke` method.
rg --type go 'func TestGroupResourceType_Revoke' --context 5
Length of output: 62
Script:
#!/bin/bash
# Description: Verify the functionality of the `Revoke` method by searching for references in test files.
# Search for any references to the `Revoke` method in Go test files.
rg 'Revoke' --type go -A 5
Length of output: 141145
Script:
#!/bin/bash
# Description: Verify the functionality of the `Revoke` method by searching for references in the test directory.
# Search for any references to the `Revoke` method in the test directory.
rg 'Revoke' --type go -A 5 --glob 'test/**'
Length of output: 43
| func (c *ConfluenceClient) GetGroupMembers( | ||
| ctx context.Context, | ||
| pageToken string, | ||
| pageSize int, | ||
| groupId string, | ||
| ) ( | ||
| []ConfluenceUser, | ||
| string, | ||
| *v2.RateLimitDescription, | ||
| error, | ||
| ) { | ||
| getUsersUrl, err := c.parse( | ||
| fmt.Sprintf(getUsersByGroupIdUrlPath, groupId), | ||
| withLimitAndOffset(pageToken, pageSize), | ||
| withQueryParameters(map[string]interface{}{ | ||
| "expand": "operations", | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| return nil, "", err | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| var resp *confluenceGroupList | ||
| if err := c.get(ctx, u, &resp); err != nil { | ||
| return nil, "", err | ||
| var response *confluenceUserList | ||
| ratelimitData, err := c.get(ctx, getUsersUrl, &response) | ||
| if err != nil { | ||
| return nil, "", ratelimitData, err | ||
| } | ||
|
|
||
| groups := resp.Results | ||
| users := response.Results | ||
|
|
||
| if len(groups) == 0 { | ||
| return groups, "", nil | ||
| if !isThereAnotherPage(response.Links) { | ||
| return users, "", ratelimitData, nil | ||
| } | ||
|
|
||
| token := incToken(pageToken, len(groups)) | ||
| token := incToken(pageToken, len(users)) | ||
|
|
||
| return groups, token, nil | ||
| return users, token, ratelimitData, nil |
There was a problem hiding this comment.
Tip
Codebase Verification
The GetGroupMembers method lacks direct unit tests.
The GetGroupMembers method in pkg/connector/client/confluence.go does not have direct unit tests in the existing test files. Ensure that comprehensive tests are added to cover this method.
- Add unit tests for the
GetGroupMembersmethod in the appropriate test file.
Analysis chain
New method looks good! Ensure comprehensive testing.
The GetGroupMembers method encapsulates the logic for retrieving group members and handles rate limit data. Ensure that the method is thoroughly tested.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the functionality of the `GetGroupMembers` method.
# Test: Search for the unit test function. Expect: Unit tests for `GetGroupMembers` method.
rg --type go 'func TestConfluenceClient_GetGroupMembers' --context 5
Length of output: 70
Script:
#!/bin/bash
# Description: Verify the existence of tests for the `GetGroupMembers` method.
# Test: Search for any references to `GetGroupMembers` in Go test files.
rg --type go 'GetGroupMembers' --context 5
Length of output: 1113
Script:
#!/bin/bash
# Description: Verify the existence of tests for the `GetGroupMembers` method.
# Test: Search for test functions in the `pkg/connector` directory.
rg --type go '^func Test' pkg/connector --context 5
Length of output: 1336
Script:
#!/bin/bash
# Description: Verify the existence of tests for the `GetGroupMembers` method.
# Extract the content of the `TestGroups` function in `pkg/connector/group_test.go`.
rg --type go 'func TestGroups' -A 50 pkg/connector/group_test.go
Length of output: 1260
| logger.Debug("confluence: user is not of type atlassian", zap.Any("user", user)) | ||
| return false | ||
| } | ||
| if len(user.Operations) == 0 { |
There was a problem hiding this comment.
We're not expanding operations in the list users call, so we don't sync any users.
GetUsersFromSearch() needs to add "expand": "operations", to its query params.
Also it looks like even after making that change, we still make some grants for users that we don't fetch. I'm not sure why.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pkg/connector/client/confluence.go (5 hunks)
- pkg/connector/client/path.go (1 hunks)
- pkg/connector/user.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- pkg/connector/client/path.go
- pkg/connector/user.go
Additional comments not posted (13)
pkg/connector/client/confluence.go (13)
Line range hint
61-75:
LGTM! Initialization logic is sound.The function correctly initializes a new
ConfluenceClientinstance and handles potential errors.
80-94: LGTM! Verification logic is sound.The function correctly verifies the current user by making an API call and handles potential errors.
99-101: LGTM! Pagination check logic is sound.The function correctly checks if there is another page of results.
103-132: LGTM! Group retrieval logic is sound.The function correctly handles API calls, pagination, and rate limit data to retrieve groups.
135-171: LGTM! Group member retrieval logic is sound.The function correctly handles API calls, pagination, and rate limit data to retrieve group members.
174-204: LGTM! Logic for adding a user to a group is sound.The function correctly handles API calls and rate limit data to add a user to a group.
206-227: LGTM! Logic for removing a user from a group is sound.The function correctly handles API calls and rate limit data to remove a user from a group.
229-235: LGTM! GET request logic is sound.The function correctly handles GET requests and rate limit data.
237-244: LGTM! POST request logic is sound.The function correctly handles POST requests and rate limit data.
246-251: LGTM! DELETE request logic is sound.The function correctly handles DELETE requests and rate limit data.
254-299: LGTM! HTTP request logic is sound.The function correctly handles HTTP requests and rate limit data.
302-309: LGTM! Non-paginated URL generation logic is sound.The function correctly generates non-paginated URLs.
312-332: LGTM! Paginated URL generation logic is sound.The function correctly generates paginated URLs.
| return nil, "", nil, err | ||
| } | ||
|
|
||
| if bag.Current() == nil { |
There was a problem hiding this comment.
@ggreer Is this how I should make sure both methods get run?
There was a problem hiding this comment.
I guess. There's no other way to keep state with this function.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pkg/connector/user_test.go (3 hunks)
Additional context used
Learnings (1)
pkg/connector/user_test.go (1)
Learnt from: jirwin PR: ConductorOne/baton-confluence#10 File: pkg/connector/user.go:51-84 Timestamp: 2024-07-30T20:10:33.777Z Learning: Unit tests for the `parsePageToken` function in `pkg/connector/user.go` should cover scenarios including valid tokens with and without specified sizes, invalid tokens, tokens with no current page, and tokens with an existing current page.
golangci-lint
pkg/connector/user_test.go
[error] 64-64: var-naming: var allIds should be allIDs
(revive)
GitHub Check: go-lint
pkg/connector/user_test.go
[failure] 64-64:
var-naming: var allIds should be allIDs (revive)
Additional comments not posted (3)
pkg/connector/user_test.go (3)
36-36: InitializepTokenwith appropriate size.The
pTokenis initialized with aSizeof 2, which is appropriate for pagination.
61-61: Update expected length ofresources.The expected length of
resourcesis updated to 6, reflecting the new pagination logic.
65-68: Ensure correct duplicate check logic.The logic to check for duplicates among
resourcesis correct and ensures that the cardinality of unique IDs is 2.
pkg/connector/user_test.go
Outdated
| require.NotEmpty(t, resources[0].Id) | ||
|
|
||
| allIds := mapset.NewSet[string]() |
There was a problem hiding this comment.
Fix variable naming convention.
The variable allIds should be renamed to allIDs to follow Go naming conventions.
- allIds := mapset.NewSet[string]()
+ allIDs := mapset.NewSet[string]()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allIds := mapset.NewSet[string]() | |
| allIDs := mapset.NewSet[string]() |
Tools
golangci-lint
[error] 64-64: var-naming: var allIds should be allIDs
(revive)
GitHub Check: go-lint
[failure] 64-64:
var-naming: var allIds should be allIDs (revive)
mgaeta
force-pushed
the
marcos/ref/rate-limits
branch
from
73fbde8 to
a06e2c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pkg/connector/user_test.go (3 hunks)
Additional context used
Learnings (1)
pkg/connector/user_test.go (1)
Learnt from: jirwin PR: ConductorOne/baton-confluence#10 File: pkg/connector/user.go:51-84 Timestamp: 2024-07-30T20:10:33.777Z Learning: Unit tests for the `parsePageToken` function in `pkg/connector/user.go` should cover scenarios including valid tokens with and without specified sizes, invalid tokens, tokens with no current page, and tokens with an existing current page.
Additional comments not posted (5)
pkg/connector/user_test.go (5)
11-11: Import statement formapsetis correct.The import statement for
mapsetis necessary for the new functionality that manages sets.
18-18: Test case description is clear.The description provides context for the test case, making it easier to understand its purpose.
36-36: Initialization ofpTokenwithSizefield is correct.Initializing the
pTokenwith aSizefield set to 2 refines the pagination behavior.
59-61: Update to the expected length ofresourcesslice is correct.Updating the expected length of the
resourcesslice from 2 to 6 ensures that the test accurately validates the pagination functionality under the new conditions.
64-68: Use ofmapsetlibrary and duplicate check is correct.Using the
mapsetlibrary to manage sets is an efficient way to handle duplicates. The code correctly checks for duplicates among the returned resources.
Rate Limits
Copying the pattern from
baton/confluence-datacenterwhere we try to forward along rate limit data instead of callingwait()to stall out for 1 second.Better Pagination
Originally,
Users.List()fetched every group, stored them in memory, and then found all of the group memberships for each group. We have to do this convoluted API dance because Confluence doesn't expose an endpoint that lists all users. One benefit of keeping everything in memory was that we can eliminate duplicate users.This refactored method uses some non-deprecated endpoints to achieve the same thing, except that it uses the pagination bag pattern to break up the API calls.Now we use the Search API to achieve the same thing. We might run into problems if a customer has more than 10,000 users, as the API docs mention that search queries return "up to 10k users".
The tests from the previous PR should still pass, expect we do not dedupe users.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests