Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Нежадная загрузка мастеров при обновлении объекта #293

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)


### Changed
1. Updated Flexberry ORM up to 7.2.0-beta01.
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,13 @@ public View GetDataObjectUpdateView(Type dataObjectType)
return _metadata[dataObjectType].UpdateView?.Clone();
}

/// <summary>
/// Возвращает информацию, должны ли мастера объекта загружаться в экономном режиме (только __PrimaryKey).
/// </summary>
/// <param name="dataObjectType">Тип объекта данных.</param>
/// <returns>Мастера должны загружаться экономно.</returns>
public bool IsMasterLightLoad(Type dataObjectType) => _metadata == null ? false : _metadata[dataObjectType]?.MasterLightLoad ?? false;

/// <summary>
/// Получает список зарегистрированных в модели типов по списку имён типов.
/// </summary>
Expand Down
Anisimova2020 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public sealed class DataObjectEdmTypeSettings
/// </summary>
public View UpdateView { get; set; }

/// <summary>
/// Whether to load object masters in LightLoaded state (load only primary key).
/// </summary>
public bool MasterLightLoad { get; set; }
Anisimova2020 marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// The list of exposed details.
/// </summary>
Expand All @@ -56,4 +61,4 @@ public sealed class DataObjectEdmTypeSettings
/// </summary>
public IDictionary<PropertyInfo, DataObjectEdmDetailSettings> PseudoDetailProperties { get; } = new Dictionary<PropertyInfo, DataObjectEdmDetailSettings>();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ public class DefaultDataObjectEdmModelBuilder : IDataObjectEdmModelBuilder
/// </summary>
private Dictionary<Type, View> UpdateViews { get; set; }

/// <summary>
/// Types for which masters should be light-loaded on updates (load only __PrimaryKey).
/// </summary>
private IEnumerable<Type> MasterLightLoadTypes { get; set; }

/// <summary>
/// Whether to load all masters in light-loaded mode on updates (load only __PrimaryKey).
/// </summary>
private bool MasterLightLoadAllTypes { get; set; }

private readonly PropertyInfo _keyProperty = Information.ExtractPropertyInfo<DataObject>(n => n.__PrimaryKey);

/// <summary>
Expand All @@ -88,7 +98,9 @@ public DefaultDataObjectEdmModelBuilder(
bool useNamespaceInEntitySetName = true,
PseudoDetailDefinitions pseudoDetailDefinitions = null,
Dictionary<Type, IEdmPrimitiveType> additionalMapping = null,
IEnumerable<KeyValuePair<Type, View>> updateViews = null)
IEnumerable<KeyValuePair<Type, View>> updateViews = null,
IEnumerable<Type> masterLightLoadTypes = null,
bool masterLightLoadAllTypes = false)
Comment on lines +101 to +103
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring constructor parameters for better maintainability

The constructor now has several optional parameters, which can make it unwieldy and harder to maintain. Consider refactoring by introducing a configuration object or applying the builder pattern to encapsulate these parameters for improved readability and extensibility.

{
_searchAssemblies = searchAssemblies ?? throw new ArgumentNullException(nameof(searchAssemblies), "Contract assertion not met: searchAssemblies != null");
_useNamespaceInEntitySetName = useNamespaceInEntitySetName;
Expand All @@ -105,6 +117,18 @@ public DefaultDataObjectEdmModelBuilder(
{
SetUpdateView(updateViews);
}

if (masterLightLoadTypes != null)
{
SetMasterLightLoadTypes(masterLightLoadTypes);

if (masterLightLoadAllTypes)
{
throw new ArgumentException("Parameters masterLightLoadAllTypes and masterLightLoadTypes can not be used together in DefaultDataObjectEdmModelBuilder.");
}
Comment on lines +125 to +128
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the grammar in the exception message

The exception message in the ArgumentException should use "cannot" instead of "can not" for proper grammar.

Apply this diff to fix the exception message:

-throw new ArgumentException("Parameters masterLightLoadAllTypes and masterLightLoadTypes can not be used together in DefaultDataObjectEdmModelBuilder.");
+throw new ArgumentException("Parameters masterLightLoadAllTypes and masterLightLoadTypes cannot be used together in DefaultDataObjectEdmModelBuilder.");
📝 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.

Suggested change
if (masterLightLoadAllTypes)
{
throw new ArgumentException("Parameters masterLightLoadAllTypes and masterLightLoadTypes can not be used together in DefaultDataObjectEdmModelBuilder.");
}
if (masterLightLoadAllTypes)
{
throw new ArgumentException("Parameters masterLightLoadAllTypes and masterLightLoadTypes cannot be used together in DefaultDataObjectEdmModelBuilder.");
}

}

this.MasterLightLoadAllTypes = masterLightLoadAllTypes;
}

/// <summary>
Expand Down Expand Up @@ -215,7 +239,12 @@ private void SetUpdateView(Type dataObjectType, View updateView)
{
if (!dataObjectType.IsSubclassOf(typeof(DataObject)))
turbcool marked this conversation as resolved.
Show resolved Hide resolved
{
throw new ArgumentException("Update view can be set only for a DataObject.", nameof(dataObjectType));
throw new ArgumentException($"Update view can be set only for a DataObject. Current type is {dataObjectType}", nameof(dataObjectType));
}

if (dataObjectType is null)
{
throw new ArgumentException("dataObjectType can not be null.", nameof(dataObjectType));
}

if (updateView is null)
Expand All @@ -232,6 +261,26 @@ private void SetUpdateView(Type dataObjectType, View updateView)
UpdateViews[dataObjectType] = updateView;
}

/// <summary>
/// Sets DataObject types for which masters will be light-loaded on updates (load only __PrimaryKey).
/// </summary>
/// <param name="masterLightLoadTypes">Types for which masters should be light-loaded.</param>
private void SetMasterLightLoadTypes(IEnumerable<Type> masterLightLoadTypes)
{
if (masterLightLoadTypes != null)
{
foreach (Type type in masterLightLoadTypes)
turbcool marked this conversation as resolved.
Show resolved Hide resolved
{
if (!type.IsSubclassOf(typeof(DataObject)))
{
throw new ArgumentException("MasterLightLoad option can be set only for a DataObject.", nameof(masterLightLoadTypes));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve clarity of the exception message

The exception message can be rephrased for better clarity: "The MasterLightLoad option can only be set for types that inherit from DataObject."

Apply this diff to enhance the message:

-throw new ArgumentException("MasterLightLoad option can be set only for a DataObject.", nameof(masterLightLoadTypes));
+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.

Suggested change
throw new ArgumentException("MasterLightLoad option can be set only for a DataObject.", nameof(masterLightLoadTypes));
throw new ArgumentException("MasterLightLoad option can only be set for types inheriting from DataObject.", nameof(masterLightLoadTypes));

}
}
Comment on lines +272 to +278
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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));
}
}


MasterLightLoadTypes = masterLightLoadTypes;
}
}

/// <summary>
/// Adds the property for exposing.
/// </summary>
Expand Down Expand Up @@ -310,6 +359,7 @@ private void AddDataObjectWithHierarchy(DataObjectEdmMetadata meta, Type dataObj
CollectionName = EntitySetNameBuilder(dataObjectType),
DefaultView = DynamicView.Create(dataObjectType, null).View,
UpdateView = updateView,
MasterLightLoad = MasterLightLoadAllTypes || (MasterLightLoadTypes?.Contains(dataObjectType) ?? false),
};

AddProperties(dataObjectType, typeSettings);
Expand Down Expand Up @@ -458,4 +508,4 @@ private string BuildEntityPropertyName(PropertyInfo propertyDataObject)
return propertyDataObject.Name;
}
}
}
}
Loading
Loading