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

Add asyncronous DataService interface. #212

Merged
merged 31 commits into from
Feb 21, 2023

Conversation

archaim
Copy link

@archaim archaim commented Nov 25, 2021

Discussion: #205

ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
Copy link
Member

@kuzkok kuzkok left a comment

Choose a reason for hiding this comment

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

Поля, как мне кажется не должны быть в интерфейсе, только методы, а поля это уже к реализации.

ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
@turbcool
Copy link
Contributor

turbcool commented Feb 28, 2022

Резюмирую все текущие замечания:

  1. Включать ли DataServiceOptions в интерфейс?
  2. Выносить ли часть методов из SQLDataService в интерфейс ISQLDataService? (т.к. класс SQLDataService слишком сильно расширяет IDataService).
    -> возможно стоит реализовать асинхронность не в IAsyncDataService, а в IAsyncSQLDataService?
  3. Проблема async + ref в методе UpdateObjects(ref DataObject[] objects). Как её решать?
    -> аналогичная проблема в методе LoadObjects(ref object State).

@turbcool turbcool force-pushed the feature-add-asyncdataservice branch from f0b0368 to 83ed095 Compare March 2, 2022 07:55
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
ICSSoft.STORMNET.Business/IAsyncDataService.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@turbcool turbcool left a comment

Choose a reason for hiding this comment

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

.

@turbcool
Copy link
Contributor

turbcool commented Nov 23, 2022

Добавил класс QueryRunner, чтобы сделать запуск запросов более понятным.

@bratchikov bratchikov marked this pull request as ready for review December 5, 2022 13:10
using ICSSoft.Services;
using ICSSoft.STORMNET.Business.Audit;
using ICSSoft.STORMNET.FunctionalLanguage;
using ICSSoft.STORMNET.FunctionalLanguage.SQLWhere;
using ICSSoft.STORMNET.Security;
using ICSSoft.STORMNET.Windows.Forms;

using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@turbcool turbcool Dec 7, 2022

Choose a reason for hiding this comment

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

Пытался поставить System в начало -> получаю SA1210

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ок, поправил настройку stylecop.

Copy link
Contributor

@turbcool turbcool Dec 7, 2022

Choose a reason for hiding this comment

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

Reordered

@@ -0,0 +1,205 @@
namespace NewPlatform.Flexberry.ORM.IntegratedTests
{
using ICSSoft.STORMNET;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sort usings, and put System first

Copy link
Contributor

Choose a reason for hiding this comment

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

Reordered


for (int i = 0; i < tableOperations.Count; i++)
{
this.tableOperations.Add(
Copy link

Choose a reason for hiding this comment

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

Предлагаю приватные поля именовать в _camelCase, чтобы не приходилось в коде обращать внимание есть или нет this перед полем. Мне при первом прочтении показалось что этот цикл не выполнится ни разу из-за того, что буквально перед ним в очень похожую переменную новый пустой словарь присваивается

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Переименовал приватные поля в _camelCase


Exception ex = null;

while (queries.Count > 0)
Copy link

Choose a reason for hiding this comment

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

Вроде список внутри цикла никак не пополняется. При этом RemoveAt имеет сложность O(n). Предлагаю заменить на foreach, и цикл будет лучше читаться и сложность на порядок уменьшится (хотя тут вряд ли именно сложность алгоритма на что-то влияет конечно)

Copy link
Contributor

Choose a reason for hiding this comment

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

Исправил


Exception ex = null;

while (queries.Count > 0)
Copy link

Choose a reason for hiding this comment

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

Аналогично

Copy link
Contributor

Choose a reason for hiding this comment

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

Исправил

}

/// <inheritdoc cref="IAsyncDataService.LoadObjectAsync(DataObject, View, bool, bool, DataObjectCache)" />
public virtual async Task LoadObjectAsync(DataObject dataObject, View dataObjectView = null, bool clearDataObject = true, bool checkExistingObject = true, DataObjectCache dataObjectCache = null)
Copy link

Choose a reason for hiding this comment

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

Метод публичный, не хватает вызова RunChangeCustomizationString по-моему

Copy link
Contributor

Choose a reason for hiding this comment

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

Добавил

try
{
Type dataObjectType = dataObject.GetType();
RunChangeCustomizationString(new Type[] { dataObjectType });
Copy link

@mao29 mao29 Dec 28, 2022

Choose a reason for hiding this comment

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

А вот тут вопрос, если подключение уже существующее передано, мы ведь не можем у него сменить строку подключения. Мне кажется тут вызов этого метода ни к чему не приведет как будто бы

Copy link
Contributor

@turbcool turbcool Feb 10, 2023

Choose a reason for hiding this comment

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

Перенёс вызов смены строки подключения перед созданием подключения.

dataObjectCache.StartCaching(false);
try
{
RunChangeCustomizationString(customizationStruct.LoadingTypes);
Copy link

Choose a reason for hiding this comment

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

Тут тоже если подключение уже существующее передано смена строки подключения на него уже не повлияет вроде бы

Copy link
Contributor

@turbcool turbcool Feb 10, 2023

Choose a reason for hiding this comment

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

Верно. Перенёс вызов перед созданием подключения.

/// <param name="query">Запрос для вычитки.</param>
/// <param name="loadingBufferSize">Ограничение на количество строк, которые будут загружены.</param>
/// <returns>Асинхронная операция (возвращает результат вычитки).</returns>
public virtual async Task<object[][]> ReadAsync(string query, int loadingBufferSize)
Copy link

Choose a reason for hiding this comment

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

А это точно публичный должен быть метод, а не protected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Согласен, лучше этот метод сделать protected. ReadAsync врят ли будет кем-то использоваться. Странно, что оригинальный метод (ReadFirst/ReadNext) - публичный.

/// <param name="loadingBufferSize">Количество строк, которые нужно загрузить в рамках текущей вычитки (используется для повторной дочитки).</param>
/// <param name="dbTransactionWrapperAsync">Содержит соединение и транзакцию, в рамках которых нужно выполнить запрос (если соединение закрыто - оно откроется).</param>
/// <returns>Асинхронная операция (возвращает результат вычитки).</returns>
public virtual async Task<object[][]> ReadByExtConnAsync(string query, int loadingBufferSize, DbTransactionWrapperAsync dbTransactionWrapperAsync)
Copy link

Choose a reason for hiding this comment

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

Аналогично насчет protected

Copy link
Contributor

Choose a reason for hiding this comment

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

Исправил

@Anisimova2020 Anisimova2020 merged commit 949c3c5 into Flexberry:develop Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants