Conversation
# Conflicts: # src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs # src/BootstrapBlazor/Services/CacheManager.cs
Contributor
Reviewer's GuideReverts the previous caching-based dynamic type creation for DataTable dynamic contexts and centralizes dynamic type reuse inside EmitHelper instead of CacheManager. Sequence diagram for dynamic type creation in DataTableDynamicContextsequenceDiagram
participant DataTableDynamicContext
participant EmitHelper
participant ModuleBuilder
DataTableDynamicContext->>DataTableDynamicContext: InternalGetColumns()
DataTableDynamicContext->>EmitHelper: CreateTypeByName(typeName, cols, parent, creatingCallback)
EmitHelper->>EmitHelper: _moduleBuilderLazy.Value
EmitHelper->>ModuleBuilder: GetType(typeName)
alt type_exists
ModuleBuilder-->>EmitHelper: existing Type
EmitHelper-->>DataTableDynamicContext: existing Type
else type_not_exists
ModuleBuilder-->>EmitHelper: null
EmitHelper->>ModuleBuilder: DefineType(typeName, attributes, parent)
alt creatingCallback_not_null
loop for_each_col
EmitHelper->>ModuleBuilder: CreateProperty(col, attributeBuilds)
end
end
ModuleBuilder-->>EmitHelper: new Type
EmitHelper-->>DataTableDynamicContext: new Type
end
DataTableDynamicContext->>DataTableDynamicContext: Columns = Utility.GetTableColumns(DynamicObjectType, cols)
DataTableDynamicContext->>DataTableDynamicContext: OnValueChanged = OnCellValueChanged
Updated class diagram for dynamic type creation and cachingclassDiagram
class DataTableDynamicContext {
+DataTable DataTable
+Action~DataTableDynamicContext,ITableColumn~ AddAttributesCallback
+Type DynamicObjectType
+IEnumerable~ITableColumn~ Columns
+EventHandler OnValueChanged
+DataTableDynamicContext(DataTable table, Action~DataTableDynamicContext,ITableColumn~ addAttributesCallback, IEnumerable~string~ invisibleColumns, IEnumerable~string~ shownColumns, IEnumerable~string~ hiddenColumns)
-IEnumerable~ITableColumn~ InternalGetColumns()
-IEnumerable~CustomAttributeBuilder~ OnColumnCreating(ITableColumn col)
-void OnCellValueChanged(object sender, EventArgs e)
-Type CreateType()
}
class EmitHelper {
<<static>>
-Lazy~ModuleBuilder~ _moduleBuilderLazy
+Type CreateTypeByName(string typeName, IEnumerable~ITableColumn~ cols, Type parent, Func~ITableColumn,IEnumerable~CustomAttributeBuilder~~ creatingCallback)
}
class CacheManager {
<<static>>
// other caching members
-Type GetDynamicObjectTypeByName(string key, IEnumerable~ITableColumn~ cols, Func~ITableColumn,IEnumerable~CustomAttributeBuilder~~ creatingCallback, out bool cached) *removed*
}
class ModuleBuilder {
+Type GetType(string className)
+TypeBuilder DefineType(string name, TypeAttributes attr, Type parent)
}
class DataTableDynamicObject {
}
class ITableColumn {
<<interface>>
+string GetFieldName()
+Type PropertyType
}
DataTableDynamicContext ..> ITableColumn : uses
DataTableDynamicContext ..> DataTableDynamicObject : dynamic parent type
DataTableDynamicContext ..> EmitHelper : calls
EmitHelper ..> ModuleBuilder : uses
EmitHelper ..> ITableColumn : creates properties
CacheManager .. EmitHelper : no longer mediates dynamic type caching
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The constructor in
DataTableDynamicContextnow callsCacheManager.GetOrCreateDynamicObjectTypeByNameandGetOrCreateType(), butGetOrCreateDynamicObjectTypeByNamehas been removed fromCacheManagerandGetOrCreateTypedoes not exist, so this code will not compile and needs to be aligned with the newEmitHelper-based logic. - In
EmitHelper.CreateTypeByName, properties are only created whencreatingCallbackis non-null; this means types created without a callback will have no properties at all, whereas previously properties were always created with optional attributes—consider restoring property creation regardless of callback presence. - The caching comment and logic around
cachedinDataTableDynamicContextare inconsistent (usingif (!cached && ...)instead of the priorif (cached && ...)semantics); once the new cache approach is finalized, ensure the condition and comments correctly reflect when attribute callbacks should run for cached vs. newly created types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The constructor in `DataTableDynamicContext` now calls `CacheManager.GetOrCreateDynamicObjectTypeByName` and `GetOrCreateType()`, but `GetOrCreateDynamicObjectTypeByName` has been removed from `CacheManager` and `GetOrCreateType` does not exist, so this code will not compile and needs to be aligned with the new `EmitHelper`-based logic.
- In `EmitHelper.CreateTypeByName`, properties are only created when `creatingCallback` is non-null; this means types created without a callback will have no properties at all, whereas previously properties were always created with optional attributes—consider restoring property creation regardless of callback presence.
- The caching comment and logic around `cached` in `DataTableDynamicContext` are inconsistent (using `if (!cached && ...)` instead of the prior `if (cached && ...)` semantics); once the new cache approach is finalized, ensure the condition and comments correctly reflect when attribute callbacks should run for cached vs. newly created types.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs" line_range="104" />
<code_context>
- }
- }
-
+ var dynamicType = EmitHelper.CreateTypeByName($"BootstrapBlazor_{nameof(DataTableDynamicContext)}_{GetHashCode()}", cols, typeof(DataTableDynamicObject), OnColumnCreating);
return dynamicType ?? throw new InvalidOperationException();
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `GetHashCode()` in the dynamic type name can lead to collisions and non-deterministic naming.
Because this type is emitted into a shared dynamic module, using `GetHashCode()` for the name makes it non-deterministic across runs and vulnerable to collisions, which will cause `DefineType` to fail when two instances share the same hash. Consider deriving the name from a stable key (e.g., the existing `cacheKey` based on column names) or a GUID, or otherwise relying on the cache you’ve added rather than per-instance hash codes.
Suggested implementation:
```csharp
// Use a deterministic name based on the cache key instead of the instance hash code.
// This avoids collisions and non-deterministic naming across runs in the shared dynamic module.
var typeName = $"BootstrapBlazor_{nameof(DataTableDynamicContext)}_{cacheKey}";
var dynamicType = EmitHelper.CreateTypeByName(typeName, cols, typeof(DataTableDynamicObject), OnColumnCreating);
```
This change assumes that a `cacheKey` string is already computed in the same scope where the dynamic type is created (e.g., in `GetOrCreateType()` or its surrounding method), as suggested by your review comment. If `cacheKey` is not yet available in this scope, you will need to:
1. Ensure that the logic that builds the cache key (based on column names) is moved or exposed so it can be used here.
2. Pass `cacheKey` into this method or local function (if `CreateType` is a local function) so it is in scope for constructing `typeName`.
If the cache key can contain characters invalid in type names, you may also want to sanitize `cacheKey` (e.g., replace non-alphanumeric characters with `_`) before building `typeName`.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Utils/EmitHelper.cs" line_range="36-43" />
<code_context>
public static Type? CreateTypeByName(string typeName, IEnumerable<ITableColumn> cols, Type? parent = null, Func<ITableColumn, IEnumerable<CustomAttributeBuilder>>? creatingCallback = null)
{
- var typeBuilder = CreateTypeBuilderByName(typeName, parent);
+ var moduleBuilder = _moduleBuilderLazy.Value;
+ var type = moduleBuilder.GetType(typeName);
+ if (type != null)
+ {
+ return type;
+ }
- foreach (var col in cols)
+ var typeBuilder = moduleBuilder.DefineType(typeName, TypeAttributes.Public, parent);
+ if (creatingCallback != null)
{
</code_context>
<issue_to_address>
**issue (bug_risk):** `CreateTypeByName` no longer creates properties when `creatingCallback` is null, which may be surprising.
Previously, properties were always created for `cols`, and a null `creatingCallback` just meant no attributes. Now the `foreach` only runs when `creatingCallback != null`, so a null callback results in no properties at all. If callers ever pass `creatingCallback == null` but still expect properties matching the columns, this is a breaking behavior change. Consider always defining the properties and only guarding the attribute application behind the callback check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets the dynamic-type emission/caching path used by DataTableDynamicContext, aiming to adjust (and partially revert) prior “use cache” logic related to emitted dynamic types.
Changes:
- Refactors
EmitHelper.CreateTypeByNameto reuse a singleModuleBuilderand return an already-created type when the sametypeNameis requested again. - Removes the dynamic-type caching helper method previously located in
CacheManager. - Modifies
DataTableDynamicContextconstructor to build a cache key and attempt to retrieve a cached emitted type.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Utils/EmitHelper.cs | Switches dynamic type emission to a shared module and adds a type lookup “cache” by type name. |
| src/BootstrapBlazor/Services/CacheManager.cs | Removes the internal dynamic-type caching helper and related Emit using. |
| src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs | Attempts to use a cache key + CacheManager API for dynamic type reuse; adjusts callback timing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7815 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34128 34117 -11
Branches 4697 4697
=========================================
- Hits 34128 34117 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7814
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Simplify dynamic type generation for data table contexts by reverting custom cache manager usage and centralizing caching within the emit helper.
Enhancements: