Add support for IDisposable #24
Conversation
📝 WalkthroughWalkthroughThe changes involve updates to GitHub Actions workflow files and enhancements to the GraphQL dependency injection framework. Specifically, the action versions for Changes
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: 3
🧹 Outside diff range and nitpick comments (3)
src/GraphQL.DI/DIObjectGraphType.cs (3)
65-73: Optimize type checking inBuildMemberInstanceExpressionIn the
BuildMemberInstanceExpressionmethod, thetypeof(IDisposable).IsAssignableFrom(typeof(TDIGraph))check is performed every time the method is called. Consider caching this result to improve performance, especially if this method is called frequently during schema building.Proposed change:
+ private static readonly bool _isDisposable = typeof(IDisposable).IsAssignableFrom(typeof(TDIGraph)); protected override LambdaExpression BuildMemberInstanceExpression(MemberInfo memberInfo) { // use an explicit type here rather than simply LambdaExpression Expression<Func<IResolveFieldContext, TDIGraph>> func; - if (typeof(IDisposable).IsAssignableFrom(typeof(TDIGraph))) { + if (_isDisposable) { func = (IResolveFieldContext context) => MemberInstanceDisposableFunc(context); } else { func = (IResolveFieldContext context) => MemberInstanceFunc(context); } return func; }
79-85: Simplify exception handling for missingRequestServicesIn
MemberInstanceFunc, you use a helper methodThrowMissingRequestServicesException()to throw an exception whenRequestServicesis null. For clarity and consistency, consider throwing the exception directly within the null-coalescing operator.Proposed change:
var graph = ActivatorUtilities.GetServiceOrCreateInstance<TDIGraph>(context.RequestServices ?? ThrowMissingRequestServicesException()); - // ... - static IServiceProvider ThrowMissingRequestServicesException() => throw new MissingRequestServicesException(); + // ... + if (context.RequestServices is null) + throw new MissingRequestServicesException(); + var graph = ActivatorUtilities.GetServiceOrCreateInstance<TDIGraph>(context.RequestServices);
Line range hint
89-98: Ensure consistent exception handling inMemberInstanceDisposableFuncIn
MemberInstanceDisposableFunc, the null check and exception throwing forRequestServicesis done inline, whereas inMemberInstanceFunc, a helper method was used. For consistency and readability, consider aligning the exception handling in both methods.Proposed change:
- var graph = (context.RequestServices ?? throw new MissingRequestServicesException()).GetService<TDIGraph>() + if (context.RequestServices is null) + throw new MissingRequestServicesException(); + var graph = context.RequestServices.GetService<TDIGraph>() ?? throw new InvalidOperationException($"Could not resolve an instance of {typeof(TDIGraph).Name} from the service provider. DI graph types that implement IDisposable must be registered in the service provider.");Additionally, consider caching the result of
typeof(TDIGraph).Nameor usingtypeof(TDIGraph).FullNamefor more detailed information in the exception message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/build.yml(1 hunks).github/workflows/publish.yml(1 hunks).github/workflows/test.yml(1 hunks)Directory.Build.props(1 hunks)src/GraphQL.DI/DIObjectGraphBase.cs(1 hunks)src/GraphQL.DI/DIObjectGraphType.cs(2 hunks)src/GraphQL.DI/GraphQLBuilderExtensions.cs(1 hunks)src/GraphQL.DI/GraphQLDIBuilderExtensions.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/GraphQL.DI/DIObjectGraphBase.cs
🔇 Additional comments (5)
.github/workflows/build.yml (1)
30-30: Verify breaking changes in actions/upload-artifact@v4
The major version upgrade from v2 to v4 could introduce breaking changes. Key changes in v4 include:
- New default artifact retention period
- Changes in artifact upload behavior
- New configuration options for compression
✅ Verification successful
Let me gather more information about the workflow file to understand the usage of upload-artifact action.
Upload artifact configuration is compatible with v4
The workflow is using upload-artifact@v4 with a simple configuration that only specifies the required parameters (name and path). The usage pattern is compatible with v4 as:
- It uses the basic configuration that remained unchanged in v4
- The artifact path pattern using glob is supported in v4
- No deprecated features or parameters are being used
- The single workflow failure on Aug 20 appears unrelated as the configuration is straightforward
The upgrade from v2 to v4 is safe in this case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any workflow runs failed after the upgrade
gh run list --workflow=build.yml --json conclusion,createdAt --jq '.[] | select(.conclusion=="failure") | .createdAt' --limit 10
Length of output: 151
Script:
#!/bin/bash
# Check the workflow file content to understand upload-artifact usage
cat .github/workflows/build.yml
Length of output: 1145
.github/workflows/publish.yml (2)
40-40: Verify breaking changes in actions/upload-artifact@v4
Similar to the build workflow, verify the compatibility of the v4 upgrade.
48-48: Verify Node.js compatibility with actions/github-script@v7
The major version upgrade from v2 to v7 includes significant changes:
- Updated Node.js runtime version
- Changes in the GitHub REST API client
- New features and potential breaking changes in the script interface
✅ Verification successful
The Node.js usage in the workflow is compatible with github-script@v7
The workflow uses basic Node.js features that are well-supported across versions:
process.versionsfor loggingrequire('fs').promisesfor file operations- Standard GitHub API calls via the
githubobject
These APIs are stable and compatible with the Node.js runtime provided by github-script@v7. The workflow has been running successfully with no failed runs detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any workflow runs failed after the upgrade
gh run list --workflow=publish.yml --json conclusion,createdAt --jq '.[] | select(.conclusion=="failure") | .createdAt' --limit 10
# Verify the Node.js API usage in all workflow files
rg -l "require\('fs'\)" .github/workflows/
Length of output: 213
Script:
#!/bin/bash
# Check the actual Node.js usage in the workflow file
cat .github/workflows/publish.yml
# Search for any other Node.js API usage patterns
rg -l "require\(|import\s+.*\s+from|new\s+URL|process\." .github/workflows/
Length of output: 2344
.github/workflows/test.yml (1)
54-54: Verify compatibility with actions/upload-artifact@v4
The update to v4 is a good improvement as it brings better performance and reliability. However, let's verify the stability of this version.
Consider adding a comment in the workflow file or documenting in the commit message why this version update was necessary, especially if there are specific features or fixes needed from v4.
✅ Verification successful
Based on the output, I can see that actions/upload-artifact@v4 is stable with recent releases (v4.4.3 as of October 2024) and includes important improvements. The most notable change is that since v4.4.0, hidden files are no longer included by default to prevent accidental credential exposure, with an opt-in include-hidden-files parameter available if needed.
Let me verify what files are being uploaded in this workflow to ensure compatibility with this breaking change.
Update to actions/upload-artifact@v4 is safe to proceed
The upgrade to v4 is compatible with your workflow as it only uploads code coverage artifacts (lcov files, XML reports, and HTML reports) which don't contain hidden files. The breaking change in v4.4.0 regarding hidden files doesn't affect your use case. The action is stable with regular maintenance (latest v4.4.3 from October 2024) and includes performance improvements for artifact uploads.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the latest releases and any breaking changes for actions/upload-artifact
# Get the latest releases and their dates
gh api repos/actions/upload-artifact/releases --jq '.[] | {tag_name: .tag_name, published_at: .published_at, body: .body}' | head -n 15
# Check if other repositories are using v4 successfully
rg -g "*.yml" -g "*.yaml" "actions/upload-artifact@v4" --stats
Length of output: 8853
Script:
#!/bin/bash
# Check the workflow file to see what files are being uploaded
cat .github/workflows/test.yml
Length of output: 3340
src/GraphQL.DI/GraphQLDIBuilderExtensions.cs (1)
32-62: LGTM: Extension methods correctly enhance DI capabilities
The AddDI extension methods are well-implemented, providing convenient ways to register DI graph types and scan assemblies for IDIObjectGraphBase<TSource> implementations. They enhance the configurability and extensibility of the GraphQL service setup.
Coverage Report
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Tests/DIObjectGraphTypeTests/DIObjectGraphTypeTestBase.cs (1)
34-44: LGTM! Consider adding XML documentation.The implementation correctly handles both registered and unregistered services. The default behavior maintains backward compatibility while the new parameter enables testing of unregistered service scenarios.
Consider adding XML documentation to describe the
registeredparameter:+ /// <summary> + /// Configures the test environment for a GraphQL type. + /// </summary> + /// <param name="instance">Whether to configure instance-level behavior.</param> + /// <param name="scoped">Whether to use scoped service provider.</param> + /// <param name="registered">Whether the service is registered in DI container.</param> protected IComplexGraphType Configure<T, TSource>(bool instance = false, bool scoped = false, bool registered = true)src/Tests/Execution/GraphQLBuilderTests.cs (1)
76-129: Consider adding error case tests.While the happy path is well tested, consider adding test cases for:
- Invalid type registrations
- Duplicate registrations
- Circular dependencies
src/Tests/DIObjectGraphTypeTests/Field.cs (1)
448-471: LGTM! Consider adding memory leak test.The tests thoroughly verify IDisposable support. Consider adding a test to ensure proper disposal to prevent memory leaks.
Example test:
[Fact] public async Task DisposableIsProperlyDisposed() { var disposed = false; _mockServiceRegister.Setup(x => x.GetService(typeof(CDisposable))) .Returns(new CDisposable(() => disposed = true)); Configure<CDisposable, object>(true); await VerifyFieldAsync("FieldTest", true, false, "hello"); // Verify disposal disposed.ShouldBeTrue(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)src/Tests/DIObjectGraphTypeTests/DIObjectGraphTypeTestBase.cs(1 hunks)src/Tests/DIObjectGraphTypeTests/Field.cs(2 hunks)src/Tests/Execution/GraphQLBuilderTests.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🔇 Additional comments (2)
src/Tests/Execution/GraphQLBuilderTests.cs (1)
26-52: LGTM! Comprehensive test coverage.
The test thoroughly validates the graph base registration requirements and ensures proper type validation.
src/Tests/DIObjectGraphTypeTests/Field.cs (1)
22-29: LGTM! Improved test coverage.
Converting to Theory with InlineData provides better coverage of both registered and unregistered scenarios.
Summary by CodeRabbit
New Features
Documentation
IDisposableinterface.Bug Fixes
DIObjectGraphTypeclass for better management of disposable resources.