Feature/no link callback #510
Conversation
When a resource is non-discoverable via any link or collection we can utilize the WithLinkNotFoundCallback option and provide the link by other means. This addition is required when you need to provide thousands of resources and making them discoverable or addiding them to some collection would result in an unusable device due to resources being too big for efectively processing them.
WalkthroughThe changes in this pull request enhance error handling across multiple methods in the Changes
Poem
Warning Rate limit exceeded@lubo-svk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 using PR comments)
Other keywords and placeholders
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: 5
🧹 Outside diff range and nitpick comments (16)
client/getResource.go (1)
50-57: LGTM! Consider enhancing error context.The implementation elegantly handles non-discoverable resources while maintaining backward compatibility. Consider wrapping errors with additional context for better debugging.
if cfg.linkNotFoundCallback != nil { link, err = cfg.linkNotFoundCallback(links, href) if err != nil { - return err + return fmt.Errorf("failed to get resource link via callback for href %v: %w", href, err) } } else { - return err + return fmt.Errorf("resource link not found for href %v: %w", href, err) }client/deleteResource.go (1)
51-58: LGTM! Consider adding documentation for the callback contract.The implementation of the
linkNotFoundCallbackmechanism is clean and maintains backward compatibility. To improve maintainability, consider documenting:
- Expected behavior of the callback
- Contract for the returned link
- Error scenarios that should be handled
Add a comment block above the if statement:
+// If the resource link is not found in the discovered links, +// attempt to recover using the linkNotFoundCallback if provided. +// The callback can either return a valid link or propagate the error. if cfg.linkNotFoundCallback != nil {client/updateResource.go (1)
53-60: LGTM! Consider adding documentation for the callback.The error handling implementation looks good and aligns with the PR objective of supporting non-discoverable resources. However, consider documenting the expected behavior and contract of the
linkNotFoundCallback.Add a comment above the if block explaining:
+// If the resource link is not found in the discovered links and a linkNotFoundCallback +// is provided, attempt to resolve the link through the callback. This enables support +// for non-discoverable resources. if cfg.linkNotFoundCallback != nil {client/createResource.go (2)
55-62: Add documentation for the new linkNotFoundCallback functionalityWhile the error handling logic is sound, it would be beneficial to document the purpose and expected behavior of the
linkNotFoundCallback. This helps other developers understand when and how to use this feature.Add a comment above the if block explaining:
+// If the resource link is not found and a callback is configured, +// attempt to retrieve the link using the callback before failing if cfg.linkNotFoundCallback != nil {
60-62: Fix indentation in the else blockThe else block's indentation is inconsistent with the if block. While this doesn't affect functionality, consistent formatting improves readability.
- } else { - return err - } + } else { + return err + }client/createResource_test.go (1)
66-87: Consider adding error scenario test casesWhile the happy path is covered, consider adding test cases for:
- Callback returning an error
- Callback returning an invalid resource link
- Nil callback behavior
Would you like me to help generate these additional test cases?
🧰 Tools
🪛 golangci-lint
[warning] 73-73: unused-parameter: parameter 'href' seems to be unused, consider removing or renaming it as _
(revive)
🪛 GitHub Check: lint
[failure] 73-73:
unused-parameter: parameter 'href' seems to be unused, consider removing or renaming it as _ (revive)client/deleteResource_test.go (2)
96-113: Consider strengthening the link callback implementation.While the test case successfully validates non-discoverable resource handling, the current callback implementation reuses any known discoverable resource's link attributes. This might not accurately represent real-world scenarios where non-discoverable resources could have different attributes.
Consider:
- Creating a dedicated test helper that returns a properly configured resource link
- Adding validation in the callback to ensure the href matches expected patterns
- Including additional test cases with different resource types
-client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { - resourceLink, _ := links.GetResourceLink(configuration.ResourceURI) - resourceLink.Href = href - return resourceLink, nil -}), +client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { + // Create a properly configured test resource link + return schema.ResourceLink{ + Href: href, + ResourceTypes: []string{types.BINARY_SWITCH}, + Interfaces: []string{interfaces.OC_IF_A, interfaces.OC_IF_BASELINE}, + Policy: &schema.Policy{}, + }, nil +}),
Line range hint
142-185: Consider extending batch deletion test coverage.The
TestClientBatchDeleteResourcesfunction could be enhanced to include test cases for:
- Batch deletion of non-discoverable resources
- Mixed deletion of discoverable and non-discoverable resources
- Error handling when using linkNotFoundCallback with batch operations
Would you like me to help generate these additional test cases?
client/updateResource_test.go (1)
102-128: Consider enhancing the non-discoverable resource test coverageThe test case is well structured and implements the new linkNotFoundCallback feature correctly. However, consider these improvements:
Add negative test cases to verify error handling:
- Invalid href in the callback
- Nil callback
- Callback returning an error
Verify that the original endpoints are preserved after the update
Here's a suggested additional test case:
{ + name: "update non-discoverable resource - callback error", + args: args{ + deviceID: deviceID, + href: nonDiscoverableResource["href"].(string), + data: map[string]interface{}{ + "value": true, + }, + opts: []client.UpdateOption{ + client.WithDiscoveryConfiguration(core.DefaultDiscoveryConfiguration()), + client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { + return schema.ResourceLink{}, fmt.Errorf("simulated callback error") + }), + }, + }, + wantErr: true, +},test/test.go (1)
270-279: Add documentation to clarify the function's purpose.The implementation looks good and aligns with the PR objectives. However, consider adding documentation to explain:
- The purpose of creating non-discoverable resources
- The significance of removing the Discoverable flag while keeping Observable
- How this differs from the default switch resource configuration
Add this documentation before the function:
+// MakeNonDiscoverableSwitchData creates test data for a non-discoverable switch resource. +// Unlike DefaultSwitchResourceLink, this removes the Discoverable flag from the policy +// bitmask while keeping the Observable flag, allowing tests to validate handling of +// non-discoverable resources. func MakeNonDiscoverableSwitchData() map[string]interface{} {client/observeResource.go (1)
236-243: Consider adding link validation after callback.The error handling for non-discoverable resources looks good, but consider validating the link returned by the callback before proceeding.
if cfg.linkNotFoundCallback != nil { link, err = cfg.linkNotFoundCallback(links, href) if err != nil { return "", err } + if link == nil { + return "", fmt.Errorf("linkNotFoundCallback returned nil link for href: %s", href) + } } else { return "", err }client/options_internal_test.go (2)
106-111: Enhance error case coverage in callback testsWhile the basic functionality is tested, the test coverage could be improved by:
- Testing error cases where the callback returns an error
- Validating behavior with different href formats
- Testing with non-empty ResourceLinks
Consider adding these test cases:
+// Test error case +foundLink, err := o.linkNotFoundCallback(nil, "invalid://href") +require.Error(t, err) +require.Empty(t, foundLink) +// Test with non-empty links +links := schema.ResourceLinks{{Href: "/existing/resource"}} +foundLink, err = o.linkNotFoundCallback(links, "/new/resource") +require.NoError(t, err) +require.Equal(t, "/new/resource", foundLink.Href)Also applies to: 159-164, 204-209, 248-253, 291-296
94-94: Consider table-driven tests for option combinationsThe tests verify basic functionality but could be more comprehensive. Consider:
- Using table-driven tests to verify different option combinations
- Validating all fields of the returned ResourceLink
- Testing the order of option application
Example improvement:
testCases := []struct { name string opts []Option want options }{ { name: "with link not found callback", opts: []Option{ WithLinkNotFoundCallback(linkNotFoundCallback), }, want: options{ linkNotFoundCallback: linkNotFoundCallback, }, }, { name: "with multiple options", opts: []Option{ WithLinkNotFoundCallback(linkNotFoundCallback), WithInterface(testIface), WithQuery(testQuery), }, want: options{ linkNotFoundCallback: linkNotFoundCallback, // ... expected values for other options }, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { var o options for _, opt := range tc.opts { o = opt.apply(o) } // Assert all fields match expected values }) }Also applies to: 147-147, 192-192, 236-236, 279-279
client/getResource_test.go (1)
59-61: Consider using a strongly-typed struct for the responseInstead of using
map[string]interface{}, consider defining and using a strongly-typed struct for better type safety and code clarity. This would make the test more maintainable and catch potential type-related issues at compile time.-var nonDiscoverableResource map[string]interface{} +type SwitchResource struct { + Href string `json:"href"` + Value bool `json:"value"` +} +var nonDiscoverableResource SwitchResourceclient/options.go (1)
327-332: Enhance documentation for error handling scenariosThe documentation should be more specific about error handling expectations. Consider adding examples of when errors should be returned and how they affect the operation.
Add these details to the documentation:
// WithLinkNotFoundCallback is used if the target of the link is not known e.g. the resource is not present on the device // or the resource is non-discoverable. // linkNotFoundCallback is a function which is called with links and href of the resource. -// It returns a link and an error. If the error is not nil, then the link is skipped. -// Otherwise the link is replaced with the returned link. +// It returns a link and an error. The callback should: +// - Return (link, nil) when a valid replacement link is found +// - Return (schema.ResourceLink{}, err) with a descriptive error when the link cannot be resolved +// - The operation will skip the link if error is returnedclient/observeResource_test.go (1)
169-237: Improve test structure and error handlingThe test function effectively validates the observation of non-discoverable resources, but there are a few improvements to consider:
- Remove unnecessary newline after the function declaration
- Fix shadowed error variable
- Use more idiomatic test assertions
- Add comprehensive cleanup
Apply these changes:
func TestObservingNonDiscoverableResource(t *testing.T) { - testDevice(t, test.DevsimName, func(ctx context.Context, t *testing.T, c *client.Client, deviceID string) { // ... code ... - defer func() { - // restore to original value - err := c.UpdateResource(ctx, deviceID, test.TestResourceSwitchesInstanceHref("1"), map[string]interface{}{ + defer func() { + // restore to original value + errRestore := c.UpdateResource(ctx, deviceID, test.TestResourceSwitchesInstanceHref("1"), map[string]interface{}{ "value": false, - }, nil, client.WithLinkNotFoundCallback(linkNotFoundCallback)) - require.NoError(t, err) + }, nil, client.WithLinkNotFoundCallback(linkNotFoundCallback)) + require.NoError(t, errRestore) }() // ... code ... - require.Equal(t, false, response.Body["value"].(bool)) + require.False(t, response.Body["value"].(bool)) // ... code ... - require.Equal(t, true, response.Body["value"].(bool)) + require.True(t, response.Body["value"].(bool))🧰 Tools
🪛 golangci-lint
171-171: File is not
gofumpt-ed(gofumpt)
170-170: unnecessary leading newline
(whitespace)
224-224: shadow: declaration of "err" shadows declaration at line 183
(govet)
214-214: bool-compare: use require.False
(testifylint)
235-235: bool-compare: use require.True
(testifylint)
🪛 GitHub Check: lint
[failure] 170-170:
unnecessary leading newline (whitespace)
[failure] 224-224:
shadow: declaration of "err" shadows declaration at line 183 (govet)
[failure] 214-214:
bool-compare: use require.False (testifylint)
[failure] 235-235:
bool-compare: use require.True (testifylint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
client/createResource.go(1 hunks)client/createResource_test.go(2 hunks)client/deleteResource.go(1 hunks)client/deleteResource_test.go(3 hunks)client/getResource.go(1 hunks)client/getResource_test.go(2 hunks)client/observeResource.go(1 hunks)client/observeResource_test.go(2 hunks)client/options.go(3 hunks)client/options_internal_test.go(10 hunks)client/updateResource.go(1 hunks)client/updateResource_test.go(3 hunks)test/test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
client/createResource_test.go
[warning] 73-73: unused-parameter: parameter 'href' seems to be unused, consider removing or renaming it as _
(revive)
client/observeResource_test.go
171-171: File is not gofumpt-ed
(gofumpt)
170-170: unnecessary leading newline
(whitespace)
224-224: shadow: declaration of "err" shadows declaration at line 183
(govet)
214-214: bool-compare: use require.False
(testifylint)
235-235: bool-compare: use require.True
(testifylint)
client/options_internal_test.go
[warning] 84-84: unused-parameter: parameter 'links' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 138-138: unused-parameter: parameter 'links' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 184-184: unused-parameter: parameter 'links' seems to be unused, consider removing or renaming it as _
(revive)
🪛 GitHub Check: lint
client/createResource_test.go
[failure] 73-73:
unused-parameter: parameter 'href' seems to be unused, consider removing or renaming it as _ (revive)
client/observeResource_test.go
[failure] 170-170:
unnecessary leading newline (whitespace)
[failure] 224-224:
shadow: declaration of "err" shadows declaration at line 183 (govet)
[failure] 214-214:
bool-compare: use require.False (testifylint)
[failure] 235-235:
bool-compare: use require.True (testifylint)
client/options_internal_test.go
[failure] 84-84:
unused-parameter: parameter 'links' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 138-138:
unused-parameter: parameter 'links' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 184-184:
unused-parameter: parameter 'links' seems to be unused, consider removing or renaming it as _ (revive)
🔇 Additional comments (17)
client/getResource.go (1)
50-54: Verify link validation in callback implementation.
The linkNotFoundCallback could potentially return invalid or malicious links. Ensure that proper link validation is implemented in the callback and documented in the interface.
client/deleteResource.go (1)
51-58: Verify thread safety of callback execution.
Since the callback operates on the shared links slice, we should ensure thread-safe access to prevent potential race conditions.
client/updateResource.go (1)
53-62: Verify the callback's contract and link validation.
The callback could potentially return an invalid or malformed link. Consider verifying:
- The contract and expectations for the callback implementation
- Validation of the returned link structure
client/createResource.go (1)
55-62: LGTM: Error handling enhancement looks good
The new error handling mechanism with linkNotFoundCallback is well-implemented:
- Maintains backward compatibility by falling back to original behavior when callback is not set
- Properly propagates errors from both paths
- Follows Go's error handling patterns
client/createResource_test.go (1)
25-25: LGTM: Required import for callback signature
The schema package import is necessary for the new LinkNotFoundCallback functionality.
client/deleteResource_test.go (2)
27-27: LGTM! Good improvements to code organization.
The split of switchID into two separate constants and the addition of the configuration import improve code readability and test isolation.
Also applies to: 56-59
61-76: LGTM! Well-structured test setup.
The test setup is properly organized with:
- Clear initialization of the test client
- Proper cleanup with deferred close
- Error handling for device ownership
- Creation of both regular and non-discoverable resources
client/updateResource_test.go (3)
26-26: LGTM: Required import for ResourceLinks type
The new import is necessary for handling resource links in the non-discoverable resource test case.
38-50: LGTM: Improved test setup structure
Good refactoring of the test setup code. Moving the context and client initialization to the beginning of the test function follows best practices by:
- Ensuring proper resource initialization before any test case
- Using appropriate timeout constants
- Implementing proper cleanup with defer statements
52-54: Verify the test helper function exists
The code uses MakeNonDiscoverableSwitchData() from the test package. Let's verify this helper function exists and is properly implemented.
✅ Verification successful
Helper function MakeNonDiscoverableSwitchData is properly implemented and used consistently
The function exists in test/test.go and is correctly implemented to create a non-discoverable switch resource with observable behavior. It's consistently used across multiple test files:
client/deleteResource_test.goclient/updateResource_test.goclient/observeResource_test.goclient/getResource_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of MakeNonDiscoverableSwitchData
# Search for the function definition
ast-grep --pattern 'func MakeNonDiscoverableSwitchData() $_'
# Search for any usage of this function to understand its context
rg "MakeNonDiscoverableSwitchData"
Length of output: 1379
test/test.go (1)
270-279: Verify test coverage for non-discoverable resources.
Let's ensure this new test utility is being used effectively in tests.
✅ Verification successful
Let me analyze the test coverage further by checking the test files where MakeNonDiscoverableSwitchData is used.
Test coverage for non-discoverable resources is comprehensive
The utility function MakeNonDiscoverableSwitchData() is effectively used across multiple test scenarios:
TestObservingNonDiscoverableResourcein observeResource_test.go validates observation of non-discoverable resourcesTestClientUpdateResourcein updateResource_test.go includes a test case for updating non-discoverable resourcesTestClientDeleteResourcein deleteResource_test.go covers deletion of non-discoverable resourcesTestClientGetResourcein getResource_test.go verifies retrieval of non-discoverable resources
Each test properly validates the CRUD operations with appropriate link handling through WithLinkNotFoundCallback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of MakeNonDiscoverableSwitchData in tests
# Expected: Find test cases validating non-discoverable resource operations
# Search for direct usage of the function
echo "Direct function usage:"
rg "MakeNonDiscoverableSwitchData\(\)" -A 5
# Search for test cases involving non-discoverable resources
echo -e "\nRelated test cases:"
rg "TestCase.*non.?discoverable" -i
Length of output: 2374
Script:
#!/bin/bash
# Check the test cases in files where MakeNonDiscoverableSwitchData is used
echo "Test cases in updateResource_test.go:"
rg "^func Test.*" client/updateResource_test.go -A 3
echo -e "\nTest cases in observeResource_test.go:"
rg "^func Test.*" client/observeResource_test.go -A 3
echo -e "\nTest cases in deleteResource_test.go:"
rg "^func Test.*" client/deleteResource_test.go -A 3
echo -e "\nTest cases in getResource_test.go:"
rg "^func Test.*" client/getResource_test.go -A 3
# Look for the actual test scenarios using non-discoverable resources
echo -e "\nTest scenarios with non-discoverable resources:"
rg "non.?discoverable" -i client/*.go -A 5
Length of output: 12630
client/observeResource.go (1)
236-243: Verify consistent behavior across resource methods.
The implementation aligns with the broader enhancement for non-discoverable resources. Let's verify that the behavior is consistent across all resource methods.
✅ Verification successful
Based on the search results, I can now generate the final response:
Consistent implementation of linkNotFoundCallback verified across resource methods
The implementation is consistent across all resource-related operations:
- GetResource: Lines checked in client/getResource.go
- CreateResource: Lines checked in client/createResource.go
- UpdateResource: Lines checked in client/updateResource.go
- DeleteResource: Lines checked in client/deleteResource.go
- ObserveResource: Lines checked in client/observeResource.go
All methods follow the same pattern:
if cfg.linkNotFoundCallback != nil {
link, err = cfg.linkNotFoundCallback(links, href)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all resource methods implement the linkNotFoundCallback consistently
# Expected: Similar error handling pattern in CreateResource, DeleteResource, UpdateResource, and GetResource
# Search for linkNotFoundCallback usage across all resource methods
ast-grep --pattern 'if $callback != nil {
$_, $err = $callback($_, $_)
if $err != nil {
return $err
}
}'
# Search for resource methods to ensure complete coverage
rg -A 5 "func.*Resource.*linkNotFoundCallback"
Length of output: 175
Script:
#!/bin/bash
# Let's try a different approach to find resource-related methods and their error handling patterns
# Search for resource-related functions and their implementations
rg -U "func.*Resource.*\{[\s\S]+?\}" client/
# Also search specifically for linkNotFoundCallback usage
rg -U "linkNotFoundCallback" client/
Length of output: 47496
client/getResource_test.go (3)
47-58: LGTM: Well-structured test setup
The setup code follows good testing practices with proper initialization, error handling, and cleanup.
122-127: LGTM: Clear and appropriate test expectations
The test expectations correctly verify both the response code and the expected payload structure for a switch resource.
110-120: Verify compatibility of reused resource link
The test reuses a link from a discoverable resource without validating if its properties (interfaces, content-formats, etc.) are compatible with the non-discoverable resource. Consider adding validation or using a more appropriate resource link as the template.
client/options.go (2)
347-347: LGTM: Consistent implementation across option structs
The linkNotFoundCallback field is consistently added to all relevant option structs, maintaining the existing patterns and structure.
Also applies to: 354-354, 361-361, 368-368, 506-506
298-300: Verify thread-safety of callback execution
Since the callback will be used in resource operations that might be concurrent, we should verify its thread-safe usage in the implementation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #510 +/- ##
==========================================
+ Coverage 74.01% 74.09% +0.07%
==========================================
Files 124 124
Lines 8741 8764 +23
==========================================
+ Hits 6470 6494 +24
Misses 1680 1680
+ Partials 591 590 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
client/createResource_test.go (1)
65-90: Consider adding more test coverage for error scenarios.While the happy path is well tested, consider adding test cases for:
- Callback returning an error
- Both links not being found
- Invalid resource link structure
This would help improve the current coverage metrics mentioned in the PR description.
Would you like me to provide example test cases for these scenarios?
client/client.go (2)
433-439: Add method documentationPlease add a documentation comment explaining:
- The purpose of this method
- The expected behavior when the link is not found
- The role of the callback parameter
- Example usage if possible
440-443: Enhance error messagesConsider making the error messages more descriptive by including context:
- return nil, schema.ResourceLink{}, err + return nil, schema.ResourceLink{}, fmt.Errorf("failed to get device %s: %w", deviceID, err)- return nil, schema.ResourceLink{}, err + return nil, schema.ResourceLink{}, fmt.Errorf("link not found callback failed for href %s: %w", href, err)- return nil, schema.ResourceLink{}, err + return nil, schema.ResourceLink{}, fmt.Errorf("resource link not found for href %s: %w", href, err)Also applies to: 450-451, 453-454
client/observeResource_test.go (2)
172-178: Add input validation to linkNotFoundCallbackThe callback implementation could be more robust by validating input parameters.
linkNotFoundCallback := func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { + if href == "" { + return schema.ResourceLink{}, fmt.Errorf("empty href provided") + } + if len(links) == 0 { + return schema.ResourceLink{}, fmt.Errorf("no resource links available") + } resourceLink, _ := links.GetResourceLink(test.TestResourceLightInstanceHref("1")) + if resourceLink.Href == "" { + return schema.ResourceLink{}, fmt.Errorf("template resource link not found") + } resourceLink.Href = href return resourceLink, nil }
169-240: Consider adding edge case testsWhile the test covers the happy path thoroughly, consider adding test cases for:
- Multiple concurrent observers
- Network disconnection scenarios
- Invalid resource data
- Resource deletion while being observed
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
client/client.go(2 hunks)client/createResource.go(2 hunks)client/createResource_test.go(2 hunks)client/deleteResource.go(2 hunks)client/getResource.go(2 hunks)client/getResource_test.go(2 hunks)client/observeResource.go(2 hunks)client/observeResource_test.go(2 hunks)client/options_internal_test.go(10 hunks)client/updateResource.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- client/createResource.go
- client/deleteResource.go
- client/getResource.go
- client/observeResource.go
- client/options_internal_test.go
- client/updateResource.go
🔇 Additional comments (7)
client/createResource_test.go (1)
73-79: LGTM! Callback implementation addresses previous issues.
The implementation properly:
- Verifies the requested link is not found
- Uses the href parameter correctly
- Handles GetResourceLink results with proper assertions
client/getResource_test.go (3)
47-58: LGTM! Well-structured test setup.
The setup code is properly organized with appropriate error handling, cleanup, and context management.
59-61: Consider adding cleanup for the non-discoverable resource.
While the resource creation is properly handled, consider adding cleanup code in the test's defer block to ensure the non-discoverable resource is deleted after the test completes. This prevents potential side effects in subsequent test runs.
105-131: LGTM! Well-structured test case for non-discoverable resources.
The test case provides good coverage for the non-discoverable resource feature with clear expectations and proper setup. After fixing the callback issues, this will be a solid test.
client/client.go (2)
34-34: LGTM!
The schema package import is correctly added and necessary for the ResourceLink type usage.
433-457: Verify error handling in callers
The method introduces a new error handling pattern with the callback. Please ensure that all callers properly handle both the regular error case and the callback error case.
client/observeResource_test.go (1)
184-187: LGTM: Comprehensive cleanup implementation
The test properly handles cleanup by:
- Deleting the created resource
- Restoring the resource state before deletion
- Using defer for reliable cleanup
- Including error checking
Also applies to: 225-231
client/getResource_test.go
Outdated
| client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { | ||
| resourceLink, ok := links.GetResourceLink(configuration.ResourceURI) | ||
| if !ok { | ||
| return schema.ResourceLink{}, fmt.Errorf("failed to get resource link: %w", err) | ||
| } | ||
| resourceLink.Href = href | ||
| return resourceLink, nil | ||
| }), |
There was a problem hiding this comment.
Fix error handling in linkNotFoundCallback.
There are two issues in the callback:
- The error check uses an undefined
errvariable instead of theokvalue fromGetResourceLink. - The resourceLink should be validated before reuse.
Apply this diff to fix the issues:
client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) {
resourceLink, ok := links.GetResourceLink(configuration.ResourceURI)
if !ok {
- return schema.ResourceLink{}, fmt.Errorf("failed to get resource link: %w", err)
+ return schema.ResourceLink{}, fmt.Errorf("resource link not found: %s", configuration.ResourceURI)
}
+ if len(resourceLink.Endpoints) == 0 {
+ return schema.ResourceLink{}, fmt.Errorf("resource link has no endpoints")
+ }
resourceLink.Href = href
return resourceLink, nil
}),📝 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.
| client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { | |
| resourceLink, ok := links.GetResourceLink(configuration.ResourceURI) | |
| if !ok { | |
| return schema.ResourceLink{}, fmt.Errorf("failed to get resource link: %w", err) | |
| } | |
| resourceLink.Href = href | |
| return resourceLink, nil | |
| }), | |
| client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { | |
| resourceLink, ok := links.GetResourceLink(configuration.ResourceURI) | |
| if !ok { | |
| return schema.ResourceLink{}, fmt.Errorf("resource link not found: %s", configuration.ResourceURI) | |
| } | |
| if len(resourceLink.Endpoints) == 0 { | |
| return schema.ResourceLink{}, fmt.Errorf("resource link has no endpoints") | |
| } | |
| resourceLink.Href = href | |
| return resourceLink, nil | |
| }), |
There was a problem hiding this comment.
@lubo-svk pls look to this suggestion. AI has right here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/options_internal_test.go (2)
78-83: Enhance test coverage of the helper functionWhile the helper function validates basic functionality, consider adding test cases for:
- Error scenarios
- Non-nil links parameter to validate behavior with actual resource links
func checkLinkNotFoundCallback(t *testing.T, callback LinkNotFoundCallback, notFoundTestLink string) { require.NotNil(t, callback.linkNotFoundCallback) + // Test with actual links + links := schema.ResourceLinks{{Href: "/other/link"}} + foundLink, err := callback.linkNotFoundCallback(links, notFoundTestLink) + require.NoError(t, err) + require.Equal(t, notFoundTestLink, foundLink.Href) + + // Test with nil links (edge case) foundLink, err := callback.linkNotFoundCallback(nil, notFoundTestLink) require.NoError(t, err) require.Equal(t, notFoundTestLink, foundLink.Href) }
91-93: Reduce code duplication by extracting common test callbackThe
linkNotFoundCallbackimplementation is identical across all test functions. Consider extracting it to a shared helper:+// Common test helper +func newTestLinkNotFoundCallback() func(_ schema.ResourceLinks, href string) (schema.ResourceLink, error) { + return func(_ schema.ResourceLinks, href string) (schema.ResourceLink, error) { + return schema.ResourceLink{Href: href}, nil + } +} func TestApplyOnGet(t *testing.T) { // ... - linkNotFoundCallback := func(_ schema.ResourceLinks, href string) (schema.ResourceLink, error) { - return schema.ResourceLink{Href: href}, nil - } + linkNotFoundCallback := newTestLinkNotFoundCallback() // ... }This would improve maintainability and reduce the risk of inconsistencies.
Also applies to: 141-143, 183-185, 224-226, 262-264
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/options_internal_test.go (1)
78-83: Enhance test coverage for linkNotFoundCallback validationWhile the helper function provides basic validation, consider adding more test cases to cover edge cases:
- Invalid href formats
- Error scenarios
- Different ResourceLink field values
func checkLinkNotFoundCallback(t *testing.T, callback LinkNotFoundCallback, notFoundTestLink string) { require.NotNil(t, callback.linkNotFoundCallback) - foundLink, err := callback.linkNotFoundCallback(nil, notFoundTestLink) - require.NoError(t, err) - require.Equal(t, notFoundTestLink, foundLink.Href) + testCases := []struct { + name string + href string + wantErr bool + }{ + {"valid href", notFoundTestLink, false}, + {"empty href", "", true}, + {"invalid href", "invalid://href", true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + foundLink, err := callback.linkNotFoundCallback(nil, tc.href) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.href, foundLink.Href) + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/options_internal_test.go(8 hunks)
🔇 Additional comments (2)
client/options_internal_test.go (2)
105-107: LGTM! Consistent implementation of linkNotFoundCallback across test functions
The callback implementation is consistent and well-integrated into each test function's options chain. The tests properly verify the callback's presence and basic functionality.
Also applies to: 155-157, 187-189, 218-220, 256-258
127-128: LGTM! Consistent validation approach using helper functions
Good use of helper functions to maintain consistency in validation across different test functions. The approach reduces code duplication and maintains a uniform testing pattern.
Also applies to: 176-179, 207-210, 237-238, 276-279
client/getResource_test.go
Outdated
| client.WithLinkNotFoundCallback(func(links schema.ResourceLinks, href string) (schema.ResourceLink, error) { | ||
| resourceLink, ok := links.GetResourceLink(configuration.ResourceURI) | ||
| if !ok { | ||
| return schema.ResourceLink{}, fmt.Errorf("failed to get resource link: %w", err) | ||
| } | ||
| resourceLink.Href = href | ||
| return resourceLink, nil | ||
| }), |
There was a problem hiding this comment.
@lubo-svk pls look to this suggestion. AI has right here.
Co-authored-by: Jozef Kralik <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
client/client.go (1)
433-456: Consider adding documentation for the new methodThe implementation looks good, but adding documentation would help users understand:
- The purpose of the method
- The expected behavior when the link is not found
- The role of the callback parameter
Add documentation above the method:
+// GetDeviceLinkForHref retrieves a device and its resource link based on the provided deviceID and href. +// If the link is not found and a callback is provided, it will attempt to resolve the link using the callback. +// Returns the device, resource link, and any error encountered. func (c *Client) GetDeviceLinkForHref(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/client.go(2 hunks)
🔇 Additional comments (1)
client/client.go (1)
440-443: Verify error handling in dependent code
The method introduces a new way of handling link-not-found scenarios. Ensure that all callers properly handle both the device retrieval error and the link-not-found error cases.
Let's verify the usage and error handling in dependent code:
Also applies to: 445-454
✅ Verification successful
Based on the results, I can now generate the final response:
Error handling is consistent across all callers
All callers of GetDeviceLinkForHref consistently handle errors by immediately checking the error return value and propagating it up the call stack. The method is used in five locations (createResource.go, deleteResource.go, getResource.go, observeResource.go, and updateResource.go), and each implements identical error handling patterns:
- All callers check
if err != nilimmediately after the call - Each propagates the error appropriately with a return statement
- No caller attempts to use the
deviceorlinkvalues when an error occurs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new method is used across the codebase
# and verify error handling patterns
# Find all calls to GetDeviceLinkForHref
echo "Checking calls to GetDeviceLinkForHref:"
rg "GetDeviceLinkForHref\(" -A 5
# Look for error handling patterns around these calls
echo -e "\nChecking error handling patterns:"
ast-grep --pattern 'if $err != nil {
$$$
}'
Length of output: 2406
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/getResource_test.go (2)
59-61: Consider using a typed struct for the resourceWhile the current implementation works, using
map[string]interface{}reduces type safety. Consider defining a struct type for the switch resource to improve type checking and code maintainability.Example struct definition:
type SwitchResource struct { Href string `json:"href"` Value bool `json:"value"` }
118-122: Optimize error handlingThe error message in the linkNotFoundCallback can be simplified.
-return schema.ResourceLink{}, fmt.Errorf("resource link not found: %s", configuration.ResourceURI) +return schema.ResourceLink{}, fmt.Errorf("resource link %s not found", configuration.ResourceURI) -return schema.ResourceLink{}, fmt.Errorf("resource link has no endpoints") +return schema.ResourceLink{}, errors.New("resource link has no endpoints")🧰 Tools
🪛 golangci-lint (1.61.0)
121-121: fmt.Errorf can be replaced with errors.New
(perfsprint)
🪛 GitHub Check: lint
[failure] 121-121:
fmt.Errorf can be replaced with errors.New (perfsprint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/getResource_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.61.0)
client/getResource_test.go
121-121: fmt.Errorf can be replaced with errors.New
(perfsprint)
🪛 GitHub Check: lint
client/getResource_test.go
[failure] 121-121:
fmt.Errorf can be replaced with errors.New (perfsprint)
🔇 Additional comments (2)
client/getResource_test.go (2)
47-58: LGTM: Well-structured test setup
The test setup follows best practices with proper initialization, error handling, and cleanup using defer statements.
105-134: LGTM: Well-implemented test case for non-discoverable resource
The test case is well-structured with clear documentation and proper validation in the linkNotFoundCallback.
🧰 Tools
🪛 golangci-lint (1.61.0)
121-121: fmt.Errorf can be replaced with errors.New
(perfsprint)
🪛 GitHub Check: lint
[failure] 121-121:
fmt.Errorf can be replaced with errors.New (perfsprint)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/getResource_test.go (1)
60-62: Consider using a more descriptive variable nameThe variable
nonDiscoverableResourcecould be more specific, e.g.,nonDiscoverableSwitchResourceto better indicate its content type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/getResource_test.go(3 hunks)
🔇 Additional comments (2)
client/getResource_test.go (2)
48-58: LGTM: Well-structured test setup
The test initialization follows best practices with proper error handling, cleanup using defer statements, and appropriate timeout context setup.
106-135: LGTM: Robust test case for non-discoverable resource
The test case is well-implemented with:
- Proper error handling in the linkNotFoundCallback
- Validation of resource link and endpoints
- Clear test expectations and assertions
The implementation aligns well with the PR objective of supporting non-discoverable resources.
|
This PR adds possibility to create, get, update, delete and observer non-discoverable resources.
Summary by CodeRabbit
Release Notes
New Features
linkNotFoundCallback) for handling missing resource links during create, update, delete, and observe operations.Bug Fixes
Tests