Нежадная загрузка мастеров при обновлении объекта #293
Conversation
turbcool
force-pushed
the
feature-odata-update-eager
branch
from
7a73cb1
to
6c8e849
Compare
|
Нужно проверить на тесте, который я добавила в develop-версию. Там хитрый батч-запрос и вот как раз мастера не меняются. Интересно, как новая логика пережуёт проблему. |
NewPlatform.Flexberry.ORM.ODataService/Model/DefaultDataObjectEdmModelBuilder.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Model/DefaultDataObjectEdmModelBuilder.cs
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Model/DefaultDataObjectEdmModelBuilder.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Model/DefaultDataObjectEdmModelBuilder.cs
Show resolved
Hide resolved
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs
Outdated
Show resolved
Hide resolved
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs
Outdated
Show resolved
Hide resolved
turbcool
force-pushed
the
feature-odata-update-eager
branch
4 times, most recently
from
ae1de43
to
173dc50
Compare
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Extensions/DataServiceExtensions.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmTypeSettings.cs
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Extensions/DataServiceExtensions.cs
Outdated
Show resolved
Hide resolved
NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmTypeSettings.cs
Show resolved
Hide resolved
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/ModifyDataTest.cs
Outdated
Show resolved
Hide resolved
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs
Outdated
Show resolved
Hide resolved
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs
Outdated
Show resolved
Hide resolved
turbcool
force-pushed
the
feature-odata-update-eager
branch
from
73ac846
to
3197b6a
Compare
NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmModel.cs
Outdated
Show resolved
Hide resolved
…oadAllTypes и masterLightLoadTypes
|
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Startups/MasterLightLoadTestStartup.cs
Outdated
Show resolved
Hide resolved
…sterLightLoadTestStartup.cs
WalkthroughThe changes in this pull request encompass updates to the Flexberry ORM ODataService, including the addition of new parameters and methods to enhance the handling of OData update requests. Key modifications include the introduction of parameters for controlling update views and loading modes, refactoring of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ODataService
participant DataObjectController
participant DataObjectEdmModelBuilder
Client->>ODataService: Send Update Request
ODataService->>DataObjectController: Handle Request
DataObjectController->>DataObjectEdmModelBuilder: Process Update
DataObjectEdmModelBuilder->>DataObjectController: Return Result
DataObjectController->>ODataService: Send Response
ODataService->>Client: Return Response
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: 13
🧹 Outside diff range and nitpick comments (11)
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Helpers/ODataTestHelper.cs (1)
23-32: Consider enhancing XML documentation for parameters.While the method has a good summary, the parameter documentation could be more descriptive to better explain their purpose and expected format.
/// <summary> /// Добавить запись об изменении ссылки у объекта в тело запроса к OData. /// </summary> - /// <param name="requestJsonData">Исходное тело запроса.</param> - /// <param name="view">Представление исходного объекта.</param> - /// <param name="model">EDM модель.</param> - /// <param name="dataObject">Новый объект данных (по ссылке).</param> - /// <param name="relationName">Ссылка на новый объект данных.</param> + /// <param name="requestJsonData">Исходное тело запроса в формате JSON.</param> + /// <param name="view">Представление исходного объекта, определяющее структуру данных.</param> + /// <param name="model">EDM модель, содержащая метаданные для OData сервиса.</param> + /// <param name="dataObject">Новый объект данных, на который будет установлена ссылка.</param> + /// <param name="relationName">Имя свойства, представляющего связь с новым объектом данных.</param> /// <returns>Новое тело запроса к OData.</returns>NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmTypeSettings.cs (1)
39-41: Enhance the XML documentation for better clarity.The documentation could be more comprehensive by including:
- Use cases for when this setting should be enabled
- Performance implications of light loading
- Relationship with the
UpdateViewproperty- Example usage
Consider expanding the documentation like this:
/// <summary> -/// Whether to load object masters in LightLoaded state (load only primary key). +/// Whether to load object masters in LightLoaded state (load only primary key). +/// When enabled, improves performance by minimizing data fetching during updates. +/// This is particularly useful for batch operations where master objects don't need +/// to be fully loaded. Works in conjunction with UpdateView to optimize loading strategy. +/// </summary> +/// <remarks> +/// Example: When updating a Task object that has a Project master, enabling this setting +/// will only load the Project's primary key instead of all its properties, reducing +/// unnecessary data transfer. +/// </remarks> /// </summary>Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Startups/MasterLightLoadTestStartup.cs (2)
21-34: Enhance class and constructor documentation.The documentation could be more detailed to help other developers understand:
- What "light-loading" means in this context
- The implications of marking a type for light-loading
- The configuration parameter's purpose and expected values
/// <summary> -/// Startup for testing MasterLightLoad configuration. -/// Differs from TestStartup that it marks <see cref="Котенок"/> type as data object for which masters should be light-loaded. +/// Startup class for testing the MasterLightLoad configuration, which implements non-greedy loading of master objects. +/// Unlike TestStartup, this class configures the <see cref="Котенок"/> type to use light-loading for its master objects, +/// meaning that master objects are loaded with minimal required fields instead of full object graphs. /// </summary> public class MasterLightLoadTestStartup : Startup { /// <summary> - /// Initialize new instance of TestStartup. + /// Initializes a new instance of the <see cref="MasterLightLoadTestStartup"/> class. /// </summary> - /// <param name="configuration">Configuration for new instance.</param> + /// <param name="configuration">The configuration settings for the application, including any custom settings for master light-loading.</param>
68-71: Consider documenting the management token registration.The token registration looks good, but its purpose and usage should be documented.
+// Register the OData route management token for dependency injection var token = builder.MapDataObjectRoute(modelBuilder); container.RegisterInstance(typeof(ManagementToken), token);CHANGELOG.md (2)
9-9: Enhance the description of master loading parameters.Consider expanding the description to better explain the purpose and impact of these parameters. For example:
-2. `masterLightLoadTypes` and `masterLightLoadAllTypes` parameters of `DefaultDataObjectEdmModelBuilder` class. They allow to change loading mode of masters during OData update requests. +2. `masterLightLoadTypes` and `masterLightLoadAllTypes` parameters of `DefaultDataObjectEdmModelBuilder` class. They allow controlling how master objects are loaded during OData update requests, enabling non-greedy loading to improve performance.🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: Did you mean “changing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...bjectEdmModelBuilder` class. They allow to change loading mode of masters during OData up...(ALLOW_TO)
Line range hint
1-1: Clarify the Fixed section entry about master loading.The entry "Fixed loading of object with crushing of already loaded masters" is unclear. Consider rewording to better explain the issue that was fixed:
-1. Fixed loading of object with crushing of already loaded masters. +1. Fixed an issue where loading objects would corrupt previously loaded master objects.🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: Did you mean “changing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ObjectEdmModelBuilder` class. It allows to change update views for data objects (update v...(ALLOW_TO)
[grammar] ~9-~9: Did you mean “changing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...bjectEdmModelBuilder` class. They allow to change loading mode of masters during OData up...(ALLOW_TO)
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs (2)
19-23: Consider enhancing the class documentation.While the documentation explains the purpose well, it would be beneficial to add:
- Prerequisites for running these tests
- Expected test environment configuration
- Description of the MasterLightLoad feature being tested
1-233: Enhance test coverage for light loading verification.While the tests cover the main scenarios (master change, property updates, and batch operations), they lack proper verification of the light loading behavior itself. Consider:
- Add negative test cases (when light loading should not occur)
- Add test cases for edge cases (null masters, circular references)
- Implement proper verification of the light loading state
This will ensure that the MasterLightLoad feature works as intended and remains stable across changes.
NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmModel.cs (1)
531-537: LGTM! Consider enhancing the documentation.The implementation is clean and follows good practices with null-safe navigation. The method aligns well with the PR objective of implementing non-greedy loading for masters.
Consider enhancing the XML documentation to clarify what "экономном режиме" (economical mode) means in practice, for example:
/// <summary> -/// Возвращает информацию, должны ли мастера объекта загружаться в экономном режиме (только __PrimaryKey). +/// Возвращает информацию, должны ли мастера объекта загружаться в экономном режиме. +/// В экономном режиме для мастеров загружается только первичный ключ (__PrimaryKey), +/// что позволяет оптимизировать загрузку данных при обновлении объекта. /// </summary>NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs (2)
751-753: Redundant null check foredmEntityinLightLoadDataObjectThe null check for
edmEntityin theLightLoadDataObjectmethod may be redundant if all callers ensure thatedmEntityis notnull. IfGetDataObjectByEdmEntityis updated to throw an exception whenedmEntityisnull, consider removing this redundant check to streamline the code.Alternatively, if you prefer defensive programming, keep the null check for extra safety.
904-904: Correct the typo in exception messageThere is a minor typo in the exception message at line 903. The phrase "can not" should be "cannot" for proper grammar.
Apply this diff to fix the typo:
throw new ArgumentNullException(nameof(edmEntity), $"{nameof(edmEntity)} can not be null."); + $"{nameof(edmEntity)} cannot be null.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- CHANGELOG.md (1 hunks)
- NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs (11 hunks)
- NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmModel.cs (1 hunks)
- NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmTypeSettings.cs (2 hunks)
- NewPlatform.Flexberry.ORM.ODataService/Model/DefaultDataObjectEdmModelBuilder.cs (7 hunks)
- Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs (1 hunks)
- Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CodeGen/NewPlatform.Flexberry.ORM.ODataService.Tests.crp (1 hunks)
- Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Helpers/ODataTestHelper.cs (1 hunks)
- Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Startups/MasterLightLoadTestStartup.cs (1 hunks)
🔥 Files not summarized due to errors (1)
- Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CodeGen/NewPlatform.Flexberry.ORM.ODataService.Tests.crp: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~9-~9: Did you mean “changing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...bjectEdmModelBuilder` class. They allow to change loading mode of masters during OData up...(ALLOW_TO)
🔇 Additional comments (6)
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Helpers/ODataTestHelper.cs (1)
23-23: LGTM! Good improvement in variable naming.The change from
objJsonМедвtoobjJsonimproves code readability by using ASCII-only characters while maintaining the same functionality.NewPlatform.Flexberry.ORM.ODataService/Model/DataObjectEdmTypeSettings.cs (1)
42-42: Property implementation looks good, verify integration.The property design is appropriate for its purpose. The boolean type and public accessibility align well with the class design pattern.
Let's verify how this property is used across the codebase:
✅ Verification successful
Let's gather more specific information about how this property is being used by examining the actual implementations.
Based on the gathered information, I can now provide a final response as the implementation details are clear.
Property is well-integrated and properly used across the codebase
The
MasterLightLoadproperty is correctly implemented and integrated:
- Used in
DataObjectControllerfor controlling master object loading behavior during updates- Configured through
DefaultDataObjectEdmModelBuilderwith support for both type-specific and global settings- Thoroughly tested in
MasterLightLoadTestwith specific test cases for different scenarios:
- Master object changes
- Master field value updates
- Integration with the service startup configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of MasterLightLoad property # and its integration with related components # Check for property usage in controllers and services echo "Checking MasterLightLoad usage in controllers and services:" rg -l "MasterLightLoad" --type cs # Check for related configuration or builder patterns echo "\nChecking for configuration setup:" ast-grep --pattern 'new DataObjectEdmTypeSettings { $$$MasterLightLoad = $_,$$$ }'Length of output: 944
Script:
#!/bin/bash # Check the actual usage of MasterLightLoad in DataObjectController echo "Checking MasterLightLoad usage in DataObjectController:" rg "MasterLightLoad" "NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs" -B 2 -A 2 # Check how it's used in the model builder echo -e "\nChecking MasterLightLoad usage in DefaultDataObjectEdmModelBuilder:" rg "MasterLightLoad" "NewPlatform.Flexberry.ORM.ODataService/Model/DefaultDataObjectEdmModelBuilder.cs" -B 2 -A 2 # Check test implementation for insights echo -e "\nChecking test implementation:" rg "MasterLightLoad" "Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs" -B 2 -A 2Length of output: 3936
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/Startups/MasterLightLoadTestStartup.cs (1)
54-61: Verify assembly references and document dependencies.The assembly configuration looks good, but consider adding validation and documentation:
var assemblies = new[] { + // Contains the Котенок entity and related types typeof(Котенок).Assembly, + // Contains application logging functionality typeof(ApplicationLog).Assembly, + // Contains user settings and preferences typeof(UserSetting).Assembly, + // Contains locking mechanism implementations typeof(Lock).Assembly, };Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/Update/MasterLightLoadTest.cs (1)
76-79: Implement verification for light loading behavior.The comment indicates that verification requires manual debugging. Consider these alternatives:
- Use reflection to verify the loading state
- Add assertions on the loaded properties to ensure only necessary data is loaded
- Implement an interceptor in the test startup to track loaded properties
Would you like me to help implement any of these verification approaches?
✅ Verification successful
Let me try a different search strategy to understand how light loading is implemented and tested in the codebase.
This should help us understand:
- How light loading is generally handled in the codebase
- If there are any existing test patterns we can follow
- What verification mechanisms are currently in place
Based on the search results, I can now provide a comprehensive response about the light loading verification:
Consider using
GetLoadedProperties()to verify light loading stateThe codebase reveals that light loading is implemented through the
MasterLightLoadconfiguration and can be verified by checking the loaded properties state. Here's how to improve the test:
- Use
GetLoadedProperties()method to verify that only the primary key is loaded for the master object- Add assertions to check that other properties are not loaded
Example implementation:
// After the response status check var loadedProps = master.GetLoadedProperties(); Assert.Single(loadedProps); // Only __PrimaryKey should be loaded Assert.Contains("__PrimaryKey", loadedProps);This approach is more reliable than manual debugging and aligns with how the framework internally handles light loading, as seen in
DataObjectController.ModifyData.cs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing implementations of light loading verification rg -A 5 "LightLoad.*[Vv]erif"Length of output: 31
Script:
#!/bin/bash # Search for light loading related tests and implementations echo "=== Searching for light loading tests ===" rg -i "lightload" --type cs -A 5 echo -e "\n=== Searching for specific test patterns ===" ast-grep --pattern 'class $_ { $$$ [Test] public void $_() { $$$ LightLoad $$$ } }'Length of output: 22062
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CodeGen/NewPlatform.Flexberry.ORM.ODataService.Tests.crp (1)
1335-1335: LGTM!The XML closing tag is properly formatted and correctly closes the
STORMNETXMLDataServiceroot element.NewPlatform.Flexberry.ORM.ODataService/Controllers/DataObjectController.ModifyData.cs (1)
992-1002: Verify the new logic for light loading of master objectsThe logic for handling master objects with light loading has been introduced:
bool masterLightLoad = _model.IsMasterLightLoad(objectType); if (masterLightLoad && !masterOwnPropsUpdated && !isAggregator) { master = LightLoadDataObject(edmMaster); } else { master = GetDataObjectByEdmEntity(edmMaster, null, dObjs, insertIntoEnd); }Ensure that this approach correctly handles all cases, especially when the master has been modified or when aggregation is involved, to prevent any unintended consequences or data inconsistencies.
Please run comprehensive tests covering various scenarios to confirm that master objects are properly loaded and updated according to the new logic.
| /// <inheritdoc/> | ||
| public override void Configure(IApplicationBuilder app, IHostingEnvironment env) | ||
| { | ||
| IUnityContainer unityContainer = UnityFactory.GetContainer(); | ||
| unityContainer.RegisterInstance(env); | ||
|
|
||
| app.UseMiddleware<ExceptionMiddleware>(); | ||
|
|
||
| app.UseMvc(builder => | ||
| { | ||
| builder.MapRoute("Lock", "api/lock/{action}/{dataObjectId}", new { controller = "Lock" }); | ||
| builder.MapFileRoute(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and route documentation.
The middleware and routing setup looks good, but consider these improvements:
- Add error handling for container resolution
- Document the purpose of each route
public override void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
- IUnityContainer unityContainer = UnityFactory.GetContainer();
+ IUnityContainer unityContainer;
+ try
+ {
+ unityContainer = UnityFactory.GetContainer();
+ }
+ catch (Exception ex)
+ {
+ throw new InvalidOperationException("Failed to initialize Unity container", ex);
+ }
unityContainer.RegisterInstance(env);
app.UseMiddleware<ExceptionMiddleware>();
app.UseMvc(builder =>
{
+ // Route for handling object locking operations
builder.MapRoute("Lock", "api/lock/{action}/{dataObjectId}", new { controller = "Lock" });
+ // Route for serving static files
builder.MapFileRoute();
});📝 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.
| /// <inheritdoc/> | |
| public override void Configure(IApplicationBuilder app, IHostingEnvironment env) | |
| { | |
| IUnityContainer unityContainer = UnityFactory.GetContainer(); | |
| unityContainer.RegisterInstance(env); | |
| app.UseMiddleware<ExceptionMiddleware>(); | |
| app.UseMvc(builder => | |
| { | |
| builder.MapRoute("Lock", "api/lock/{action}/{dataObjectId}", new { controller = "Lock" }); | |
| builder.MapFileRoute(); | |
| }); | |
| /// <inheritdoc/> | |
| public override void Configure(IApplicationBuilder app, IHostingEnvironment env) | |
| { | |
| IUnityContainer unityContainer; | |
| try | |
| { | |
| unityContainer = UnityFactory.GetContainer(); | |
| } | |
| catch (Exception ex) | |
| { | |
| throw new InvalidOperationException("Failed to initialize Unity container", ex); | |
| } | |
| unityContainer.RegisterInstance(env); | |
| app.UseMiddleware<ExceptionMiddleware>(); | |
| app.UseMvc(builder => | |
| { | |
| // Route for handling object locking operations | |
| builder.MapRoute("Lock", "api/lock/{action}/{dataObjectId}", new { controller = "Lock" }); | |
| // Route for serving static files | |
| builder.MapFileRoute(); | |
| }); |
| // Set MasterLightLoad property for this DataObject | ||
| var masterLightLoadTypes = new List<Type> { typeof(Котенок) }; | ||
| var modelBuilder = new DefaultDataObjectEdmModelBuilder(assemblies, false, pseudoDetailDefinitions, masterLightLoadTypes: masterLightLoadTypes); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating masterLightLoadTypes configuration.
The light-load configuration looks correct, but consider adding validation to ensure the type exists in the referenced assemblies.
-// Set MasterLightLoad property for this DataObject
-var masterLightLoadTypes = new List<Type> { typeof(Котенок) };
-var modelBuilder = new DefaultDataObjectEdmModelBuilder(assemblies, false, pseudoDetailDefinitions, masterLightLoadTypes: masterLightLoadTypes);
+// Configure light-loading for master objects of Котенок type
+var masterLightLoadTypes = new List<Type> { typeof(Котенок) };
+
+// Validate that the light-load type exists in the referenced assemblies
+if (!assemblies.Any(a => a.GetTypes().Contains(typeof(Котенок))))
+{
+ throw new InvalidOperationException($"Type {typeof(Котенок).FullName} not found in the referenced assemblies");
+}
+
+var modelBuilder = new DefaultDataObjectEdmModelBuilder(
+ assemblies,
+ false,
+ pseudoDetailDefinitions,
+ masterLightLoadTypes: masterLightLoadTypes);📝 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.
| // Set MasterLightLoad property for this DataObject | |
| var masterLightLoadTypes = new List<Type> { typeof(Котенок) }; | |
| var modelBuilder = new DefaultDataObjectEdmModelBuilder(assemblies, false, pseudoDetailDefinitions, masterLightLoadTypes: masterLightLoadTypes); | |
| // Configure light-loading for master objects of Котенок type | |
| var masterLightLoadTypes = new List<Type> { typeof(Котенок) }; | |
| // Validate that the light-load type exists in the referenced assemblies | |
| if (!assemblies.Any(a => a.GetTypes().Contains(typeof(Котенок)))) | |
| { | |
| throw new InvalidOperationException($"Type {typeof(Котенок).FullName} not found in the referenced assemblies"); | |
| } | |
| var modelBuilder = new DefaultDataObjectEdmModelBuilder( | |
| assemblies, | |
| false, | |
| pseudoDetailDefinitions, | |
| masterLightLoadTypes: masterLightLoadTypes); |
| @@ -6,6 +6,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| ### Added | |||
| 1. `updateViews` parameter of `DefaultDataObjectEdmModelBuilder` class. It allows to change update views for data objects (update view is used for loading a data object during OData update requests). | |||
| 2. `masterLightLoadTypes` and `masterLightLoadAllTypes` parameters of `DefaultDataObjectEdmModelBuilder` class. They allow to change loading mode of masters during OData update requests. | |||
There was a problem hiding this comment.
Fix grammatical issue in the parameter description.
The phrase "allow to change" is grammatically incorrect. Consider rewording:
-They allow to change loading mode of masters during OData update requests.
+They allow changing the loading mode of masters during OData update requests.📝 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.
| 2. `masterLightLoadTypes` and `masterLightLoadAllTypes` parameters of `DefaultDataObjectEdmModelBuilder` class. They allow to change loading mode of masters during OData update requests. | |
| 2. `masterLightLoadTypes` and `masterLightLoadAllTypes` parameters of `DefaultDataObjectEdmModelBuilder` class. They allow changing the loading mode of masters during OData update requests. |
🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: Did you mean “changing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...bjectEdmModelBuilder` class. They allow to change loading mode of masters during OData up...(ALLOW_TO)
| string[] changesets = new[] // Важно, чтобы сначала шёл мастер, потом объект, имеющий на него ссылку. | ||
| { | ||
| CreateChangeset( | ||
| $"{baseUrl}/{args.Token.Model.GetEdmEntitySet(typeof(Кошка)).Name}", | ||
| кошка.ToJson(кошкаDynamicView, args.Token.Model), | ||
| кошка), | ||
| CreateChangeset( | ||
| $"{baseUrl}/{args.Token.Model.GetEdmEntitySet(typeof(Котенок)).Name}", | ||
| котенокJsonData, | ||
| котенок), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the changeset ordering requirement more robust.
The comment indicates that master updates must precede dependent object updates. Consider:
- Add validation to enforce this ordering
- Document the requirement more prominently
- Add a test case that verifies the behavior when the order is incorrect
Example validation:
private void ValidateChangesetOrder(string[] changesets)
{
// Verify that master updates come before dependent updates
var masterIndex = Array.FindIndex(changesets, c => c.Contains("/Кошка"));
var dependentIndex = Array.FindIndex(changesets, c => c.Contains("/Котенок"));
if (masterIndex > dependentIndex && masterIndex != -1 && dependentIndex != -1)
{
throw new InvalidOperationException("Master updates must precede dependent updates in changesets");
}
}| string[] котенокPropertiesNames = | ||
| { | ||
| Information.ExtractPropertyPath<Котенок>(x => x.__PrimaryKey), | ||
| Information.ExtractPropertyPath<Котенок>(x => x.Глупость), | ||
| }; | ||
| var котенокDynamicView = new View(new ViewAttribute("котенокDynamicView", котенокPropertiesNames), typeof(Котенок)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate view creation logic.
The view creation logic is duplicated across all test methods. Consider:
- Extract view creation to helper methods
- Create reusable view constants
Example refactoring:
private static class TestViews
{
public static View GetКотенокView(bool includeMaster = false)
{
var properties = new[]
{
Information.ExtractPropertyPath<Котенок>(x => x.__PrimaryKey),
Information.ExtractPropertyPath<Котенок>(x => x.Глупость),
};
if (includeMaster)
{
properties = properties.Concat(new[]
{
Information.ExtractPropertyPath<Котенок>(x => x.Кошка),
}).ToArray();
}
return new View(new ViewAttribute("котенокDynamicView", properties), typeof(Котенок));
}
}Also applies to: 215-221
| foreach (Type type in masterLightLoadTypes) | ||
| { | ||
| if (!type.IsSubclassOf(typeof(DataObject))) | ||
| { | ||
| throw new ArgumentException("MasterLightLoad option can be set only for a DataObject.", nameof(masterLightLoadTypes)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check for elements in masterLightLoadTypes
There is no check for null elements within the masterLightLoadTypes collection. If a null Type is encountered, it could lead to a NullReferenceException. Consider adding a null check for each element.
Apply this diff to include the null check:
foreach (Type type in masterLightLoadTypes)
{
+ if (type is null)
+ {
+ throw new ArgumentException("Type in masterLightLoadTypes cannot be null.", nameof(masterLightLoadTypes));
+ }
if (!type.IsSubclassOf(typeof(DataObject)))
{
throw new ArgumentException("MasterLightLoad option can only be set for types inheriting from DataObject.", nameof(masterLightLoadTypes));
}
}📝 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.
| foreach (Type type in masterLightLoadTypes) | |
| { | |
| if (!type.IsSubclassOf(typeof(DataObject))) | |
| { | |
| throw new ArgumentException("MasterLightLoad option can be set only for a DataObject.", nameof(masterLightLoadTypes)); | |
| } | |
| } | |
| foreach (Type type in masterLightLoadTypes) | |
| { | |
| if (type is null) | |
| { | |
| throw new ArgumentException("Type in masterLightLoadTypes cannot be null.", nameof(masterLightLoadTypes)); | |
| } | |
| if (!type.IsSubclassOf(typeof(DataObject))) | |
| { | |
| throw new ArgumentException("MasterLightLoad option can be set only for a DataObject.", nameof(masterLightLoadTypes)); | |
| } | |
| } |
| lightView.Details = new DetailInView[0]; | ||
|
|
There was a problem hiding this comment.
Potential issue with clearing DetailsInView
In the LoadDataObject method, you're cloning the view and then clearing its details:
DetailInView[] lightViewDetails = lightView.Details;
lightView.Details = new DetailInView[0];If lightView.Details is expected to be a modifiable collection, replacing it with a new empty array might not reflect in all parts of the code that use the original collection. Consider clearing the existing collection instead of assigning a new array.
Apply this diff to clear the collection:
- lightView.Details = new DetailInView[0];
+ lightView.Details.Clear();Ensure that Details is a collection type that supports the Clear method.
| if (dataObjectFromCache.GetStatus(false) == ObjectStatus.UnAltered | ||
| && dataObjectFromCache.GetLoadingState() != LoadingState.Loaded) | ||
| { | ||
| // Если объект не новый и не загружен целиком (начиная с [email protected]). | ||
| if (dataObjectFromCache.GetStatus(false) == ObjectStatus.UnAltered | ||
| && dataObjectFromCache.GetLoadingState() != LoadingState.Loaded) | ||
| // Для обратной совместимости сравним перечень загруженных свойств и свойств в представлении. | ||
| /* Данный код срабатывает, например, если в кэше был объект, который загрузился только на уровне первичного ключа. | ||
| * | ||
| * Данный код также срабатывает в следующей ситуации: есть класс А, у него детейл Б, у которого есть наследник В. | ||
| * При загрузке объекта класса А подгрузятся его детейлы, однако они будут подгружены по представлению, которое соответствует классу Б, даже если детейлы класса В. | ||
| * Таким образом, в кэше окажутся объекты класса В, которые загружены только по свойствам Б. Раз не все свойства подгружены, то состояние LightLoaded. | ||
| * Догружать необходимо только те свойства, что ещё не загружались (потому что загруженные уже могут быть изменены). |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize loading state checks
In the condition checking the loading state of dataObjectFromCache:
if (dataObjectFromCache.GetStatus(false) == ObjectStatus.UnAltered
&& dataObjectFromCache.GetLoadingState() != LoadingState.Loaded)
{
// Additional logic
}Consider combining the status and loading state checks into a single method or property if possible, to simplify the condition and improve code readability.
Refactor the condition into a method like NeedsLoading(dataObjectFromCache) for clarity.
| private DataObject GetDataObjectByEdmEntity(EdmEntityObject edmEntity, object key, List<DataObject> dObjs, bool endObject = false) | ||
| { | ||
| if (edmEntity == null) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
Ensure consistent null handling for edmEntity parameter
In the GetDataObjectByEdmEntity method, when edmEntity is null, the method returns null without any error or exception. Considering that edmEntity is essential for data object retrieval, it would be better to throw an ArgumentNullException to prevent silent failures and aid in debugging.
Apply this diff to enforce null checking:
private DataObject GetDataObjectByEdmEntity(EdmEntityObject edmEntity, object key, List<DataObject> dObjs, bool endObject = false)
{
- if (edmEntity == null)
- {
- return null;
- }
+ if (edmEntity == null)
+ {
+ throw new ArgumentNullException(nameof(edmEntity));
+ }📝 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.
| private DataObject GetDataObjectByEdmEntity(EdmEntityObject edmEntity, object key, List<DataObject> dObjs, bool endObject = false) | |
| { | |
| if (edmEntity == null) | |
| { | |
| return null; | |
| private DataObject GetDataObjectByEdmEntity(EdmEntityObject edmEntity, object key, List<DataObject> dObjs, bool endObject = false) | |
| { | |
| if (edmEntity == null) | |
| { | |
| throw new ArgumentNullException(nameof(edmEntity)); | |
| } |
| private object GetKey(EdmEntityObject edmEntity) | ||
| { | ||
| if (edmEntity == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(edmEntity), $"{nameof(edmEntity)} can not be null."); | ||
| } | ||
|
|
||
| object key; | ||
|
|
||
| // Получим значение ключа. | ||
| IEdmEntityType entityType = (IEdmEntityType)edmEntity.ActualEdmType; | ||
| IEnumerable<IEdmProperty> entityProps = entityType.Properties(); | ||
| var keyProperty = entityProps.FirstOrDefault(prop => prop.Name == _model.KeyPropertyName); | ||
| edmEntity.TryGetPropertyValue(keyProperty.Name, out key); | ||
|
|
||
| return key; | ||
| } |
There was a problem hiding this comment.
Handle potential NullReferenceException in GetKey method
In the GetKey method, if the key property is not found, keyProperty will be null, leading to a possible NullReferenceException when accessing keyProperty.Name at line 912. It's advisable to check if keyProperty is null and throw a meaningful exception if the key property is missing.
Apply this diff to handle the null case:
var keyProperty = entityProps.FirstOrDefault(prop => prop.Name == _model.KeyPropertyName);
+ if (keyProperty == null)
+ {
+ throw new InvalidOperationException($"Key property '{_model.KeyPropertyName}' not found in the entity of type '{entityType.FullTypeName()}'.");
+ }
edmEntity.TryGetPropertyValue(keyProperty.Name, out key);📝 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.
| private object GetKey(EdmEntityObject edmEntity) | |
| { | |
| if (edmEntity == null) | |
| { | |
| throw new ArgumentNullException(nameof(edmEntity), $"{nameof(edmEntity)} can not be null."); | |
| } | |
| object key; | |
| // Получим значение ключа. | |
| IEdmEntityType entityType = (IEdmEntityType)edmEntity.ActualEdmType; | |
| IEnumerable<IEdmProperty> entityProps = entityType.Properties(); | |
| var keyProperty = entityProps.FirstOrDefault(prop => prop.Name == _model.KeyPropertyName); | |
| edmEntity.TryGetPropertyValue(keyProperty.Name, out key); | |
| return key; | |
| } | |
| private object GetKey(EdmEntityObject edmEntity) | |
| { | |
| if (edmEntity == null) | |
| { | |
| throw new ArgumentNullException(nameof(edmEntity), $"{nameof(edmEntity)} can not be null."); | |
| } | |
| object key; | |
| // Получим значение ключа. | |
| IEdmEntityType entityType = (IEdmEntityType)edmEntity.ActualEdmType; | |
| IEnumerable<IEdmProperty> entityProps = entityType.Properties(); | |
| var keyProperty = entityProps.FirstOrDefault(prop => prop.Name == _model.KeyPropertyName); | |
| if (keyProperty == null) | |
| { | |
| throw new InvalidOperationException($"Key property '{_model.KeyPropertyName}' not found in the entity of type '{entityType.FullTypeName()}'."); | |
| } | |
| edmEntity.TryGetPropertyValue(keyProperty.Name, out key); | |
| return key; | |
| } |
|
#290, пункт 2. Мержить после #292.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation