featute/ChangeLog #6

Merged
on.nemtina merged 25 commits from featute/ChangeLog into master 2024-12-09 17:44:48 +05:00
Owner
https://project.ddrilling.ru/projects/persistance-service/work_packages/400/activity
on.nemtina added 12 commits 2024-12-03 11:37:19 +05:00
on.nemtina requested review from ng.frolov 2024-12-03 11:37:25 +05:00
ng.frolov was assigned by on.nemtina 2024-12-03 11:37:29 +05:00
on.nemtina changed title from featute/ChangeLog to WIP: featute/ChangeLog 2024-12-03 11:37:36 +05:00
ng.frolov requested review from rs.efremov 2024-12-03 15:42:47 +05:00
ng.frolov requested changes 2024-12-03 16:19:44 +05:00
Dismissed
@ -0,0 +26,4 @@
[FromBody] DataWithWellDepthAndSectionDto dto,
CancellationToken token)
{
var userId = User.GetUserId<Guid>();
Collaborator

Тут Да же проблема что и с сообщениями. Id системы, которая нам эти данные отправила, сюда не очень подходит

Тут Да же проблема что и с сообщениями. Id системы, которая нам эти данные отправила, сюда не очень подходит
Author
Owner

Решили пока оставить, как есть

Решили пока оставить, как есть
@ -0,0 +49,4 @@
public async Task<IActionResult> Delete(Guid id, CancellationToken token)
{
var userId = User.GetUserId<Guid>();
var result = await repository.MarkAsDeleted(userId, [id], token);
Collaborator

Если метод repository.MarkAsDeleted вернет 0 (удаляемая запись отсутствует), то методы delete должны возвращать NoContent

Если метод repository.MarkAsDeleted вернет 0 (удаляемая запись отсутствует), то методы delete должны возвращать NoContent
@ -0,0 +11,4 @@
{
private const string BaseRoute = "/api/ChangeLog";
//[Get($"{BaseRoute}/current")]
Collaborator

лишние комменты

лишние комменты
@ -0,0 +6,4 @@
/// <summary>
/// Часть записи, описывающая изменение
/// </summary>
public interface IChangeLog
Collaborator

Кажется этот интерфейс уже не нужен

Кажется этот интерфейс уже не нужен
Collaborator

Он все еще не нужен..

Он все еще не нужен..
@ -32,3 +32,3 @@
/// <summary>
/// Id состояния
/// Ключ следующей записи
Collaborator

Заменившей

Заменившей
@ -0,0 +11,4 @@
/// </summary>
public class DataWithWellDepthAndSectionDto
{
public DataWithWellDepthAndSectionDto()
Collaborator

пустые конструкторы не нужны

пустые конструкторы не нужны
@ -0,0 +39,4 @@
/// <summary>
/// Объект записи
/// </summary>
public required IDictionary<string, object> Value { get; set; } = default!;
Collaborator

= default!; - лишнее

= default!; - лишнее
@ -0,0 +3,4 @@
/// <summary>
/// Часть записи описывающая изменение
/// </summary>
public interface IChangeLogDto
Collaborator

А зачем он тогда нужен?

А зачем он тогда нужен?
@ -0,0 +5,4 @@
using System.Threading.Tasks;
namespace Persistence.Models;
public class ProcessMapRotorDto : IChangeLogDto
Collaborator

delete

delete
@ -0,0 +9,4 @@
/// <summary>
/// Кол-во записей пропущенных с начала таблицы в запросе от api
/// </summary>
public int? Skip { get; set; }
Collaborator

Skip/Take - используются только с PaginationContainer. Поэтому предлагаю сделать из обязательными, но со значениями по умолчанию.

Skip/Take - используются только с PaginationContainer. Поэтому предлагаю сделать из обязательными, но со значениями по умолчанию.
@ -0,0 +3,4 @@
/// <summary>
/// Запрос для фильтрации данных по секции и глубине
/// </summary>
public class SectionPartRequest : Request
Collaborator

Возможно тут было бы проще без наследования

Возможно тут было бы проще без наследования
@ -53,3 +20,1 @@
/// <param name="token"></param>
/// <returns></returns>
Task<int> ClearAndInsertRange(int idUser, IEnumerable<TDto> dtos, CancellationToken token);
Task<int> InsertRange(Guid idUser, Guid idDiscriminator, IEnumerable<DataWithWellDepthAndSectionDto> dtos, CancellationToken token);
Collaborator

idUser -> idAuthor, Такое имя позволит не читать комментарий. И совпадает с названием свойства Dto

idUser -> idAuthor, Такое имя позволит не читать комментарий. И совпадает с названием свойства Dto
@ -86,3 +56,3 @@
/// <param name="token"></param>
/// <returns></returns>
Task<IEnumerable<TDto>> GetCurrent(DateTimeOffset moment, CancellationToken token);
Task<int> UpdateRange(Guid idUser, Guid idDiscriminator, IEnumerable<DataWithWellDepthAndSectionDto> dtos, CancellationToken token);
Collaborator

DataWithWellDepthAndSectionDto содержит Guid Id, Поэтому Guid idDiscriminator не нужен.

DataWithWellDepthAndSectionDto содержит Guid Id, Поэтому Guid idDiscriminator не нужен.
@ -89,0 +75,4 @@
/// <param name="dateEnd"></param>
/// <param name="token"></param>
/// <returns></returns>
Task<IEnumerable<ChangeLogDto>> GetChangeLogForDate(Guid idDiscriminator, DateTimeOffset dateBegin, DateTimeOffset dateEnd, CancellationToken token);
Collaborator

Лучше ForDate -> ForInterval
Кстати в вместо аргументов dateBegin, dateEnd можно использовать DatesRangeDto

Лучше ForDate -> ForInterval Кстати в вместо аргументов dateBegin, dateEnd можно использовать DatesRangeDto
on.nemtina added 1 commit 2024-12-03 17:05:58 +05:00
on.nemtina added 1 commit 2024-12-03 17:36:52 +05:00
on.nemtina changed title from WIP: featute/ChangeLog to featute/ChangeLog 2024-12-04 11:45:43 +05:00
on.nemtina added 1 commit 2024-12-05 11:01:10 +05:00
on.nemtina added 2 commits 2024-12-05 11:30:21 +05:00
on.nemtina added 1 commit 2024-12-05 12:20:47 +05:00
ng.frolov requested changes 2024-12-05 14:05:10 +05:00
Dismissed
@ -0,0 +20,4 @@
}
[HttpPost]
[ProducesResponseType(typeof(int), (int)HttpStatusCode.OK)]
Collaborator

Не тот хттп код
И дальше тоже есть.

Не тот хттп код И дальше тоже есть.
@ -0,0 +22,4 @@
[HttpPost]
[ProducesResponseType(typeof(int), (int)HttpStatusCode.OK)]
public async Task<IActionResult> Add(
Guid idDiscriminator,
Collaborator

idDiscriminator лучше сделать частью route

idDiscriminator лучше сделать частью route
@ -0,0 +167,4 @@
var result = await repository.GetDatesRange(idDiscriminator, token);
if(result is null)
return NoContent();
Collaborator

Нужно добавить все коды возврата в аттрибуты метода, чтобы в сваггере они отображались и пользователи знали что при обработке ответа такой код может прилететь и им его как-то надо обработать.

Нужно добавить все коды возврата в аттрибуты метода, чтобы в сваггере они отображались и пользователи знали что при обработке ответа такой код может прилететь и им его как-то надо обработать.
@ -0,0 +12,4 @@
/// <summary>
/// Ключ записи
/// </summary>
[Key, Column("Id")]
Collaborator

Договаривались в моделях БД не указывать имя колонки, чтобы оно совпадало с именем свойства.
А чтобы модель визуально оставалась узнаваемой как модель решили указывать атрибут комментария..

Договаривались в моделях БД не указывать имя колонки, чтобы оно совпадало с именем свойства. А чтобы модель визуально оставалась узнаваемой как модель решили указывать атрибут комментария..
@ -0,0 +24,4 @@
}
[Fact]
public async Task ClearAndInsertRange()
Collaborator

Этот кейс проверяет на пустой базе и не охватывает логику отмечания существующих в БД данных как удаленные.

Этот кейс проверяет на пустой базе и не охватывает логику отмечания существующих в БД данных как удаленные.
@ -0,0 +20,4 @@
foreach (var dto in dtos)
{
var entity = CreateEntityFromDto(idAuthor, idDiscriminator, dto);
db.Set<ChangeLog>().Add(entity);
Collaborator

db.Set() хоть и не дорогой но не бесплатный:) Получать его для каждой dto как-то не оправданно.

db.Set<ChangeLog>() хоть и не дорогой но не бесплатный:) Получать его для каждой dto как-то не оправданно.
@ -0,0 +32,4 @@
{
var query = db.Set<ChangeLog>().Where(s => ids.Contains(s.Id));
var entities = await query.ToArrayAsync(token);
Collaborator

Стоит проверить, что количество записей совпадает с количеством ids. Если не совпадает - исключение

Стоит проверить, что количество записей совпадает с количеством ids. Если не совпадает - исключение
@ -0,0 +50,4 @@
return result;
}
private async Task<int> Clear(Guid idEditor, IEnumerable<ChangeLog> entities, CancellationToken token)
Collaborator

немного смущает название

немного смущает название
@ -0,0 +54,4 @@
{
var updateTime = DateTimeOffset.UtcNow;
foreach (var entity in entities)
Collaborator

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

Думаю тут будет разумно проверить, что помечаемые записи еще не устарели. И если мы собираемся отредактировать устаревшее, то падаем в исключение.
@ -0,0 +67,4 @@
{
var result = 0;
using var transaction = await db.Database.BeginTransactionAsync(token);
try
Collaborator

try-catch тут и дальше не нужен. Rollback ролбэк будет вызван финализатором транзакции, если не был вызван комит

try-catch тут и дальше не нужен. Rollback ролбэк будет вызван финализатором транзакции, если не был вызван комит
@ -0,0 +100,4 @@
var updatedEntity = updatedEntities.GetValueOrDefault(dto.Id);
if(updatedEntity is null)
{
throw new ArgumentNullException($"Entity with id = {dto.Id} doesn't exist in Db", nameof(dto));
Collaborator

Не тот тип эксепшена.
Лучше использовать ArgumentException, а в middleware его перехватить и вернуть пользователю 400

Не тот тип эксепшена. Лучше использовать ArgumentException, а в middleware его перехватить и вернуть пользователю 400
@ -0,0 +136,4 @@
return result;
}
private IQueryable<ChangeLog> BuildQuery(Guid idDiscriminator, DateTimeOffset momentUtc, SectionPartRequest request)
Collaborator
private IQueryable<ChangeLog> MakeReadQuery(Guid idDiscriminator, DateTimeOffset momentUtc){...}
private IQueryable<ChangeLog> ApplyFilter(IQueryable<ChangeLog> query,  SectionPartRequest request){...}

"Build" - намекает на наличие билдера, а его нет. (можно подумать чтобы его завести:))

``` private IQueryable<ChangeLog> MakeReadQuery(Guid idDiscriminator, DateTimeOffset momentUtc){...} private IQueryable<ChangeLog> ApplyFilter(IQueryable<ChangeLog> query, SectionPartRequest request){...} ``` "Build" - намекает на наличие билдера, а его нет. (можно подумать чтобы его завести:))
@ -0,0 +140,4 @@
{
var query = db.Set<ChangeLog>()
.Where(e => e.IdDiscriminator == idDiscriminator)
.Where(e => e.Creation <= momentUtc)
Collaborator

Мы не доверяем пользователям и все равно приводим к UTC

Мы не доверяем пользователям и все равно приводим к UTC
@ -0,0 +163,4 @@
{
var query = db.Set<ChangeLog>().Where(s => s.IdDiscriminator == idDiscriminator);
var min = new DateTimeOffset(dateBegin.Year, dateBegin.Month, dateBegin.Day, 0, 0, 0, TimeSpan.Zero);
Collaborator

Тут ошибка в часовых поясах.
var min = new DateTimeOffset(dateBegin.ToUniversalTime().Date, TimeSpan.Zero);

Тут ошибка в часовых поясах. `var min = new DateTimeOffset(dateBegin.ToUniversalTime().Date, TimeSpan.Zero);`
@ -0,0 +181,4 @@
private async Task<PaginationContainer<DataWithWellDepthAndSectionDto>> BuildPaginationContainer(IQueryable<ChangeLog> query, PaginationRequest request, CancellationToken token)
{
var result = new PaginationContainer<DataWithWellDepthAndSectionDto>
Collaborator

result лучше собирать, когда для него уже есть все данные

result лучше собирать, когда для него уже есть все данные
@ -0,0 +206,4 @@
.Take(result.Take)
.ToArrayAsync(token);
var dtos = entities.Select(e => e.Adapt<DataWithWellDepthAndSectionDto>());
Collaborator

Конвертацию лучше вынести в метод. Так связь с маппером слабже и преобразование единообразное.

Конвертацию лучше вынести в метод. Так связь с маппером слабже и преобразование единообразное.
@ -0,0 +242,4 @@
{
var entity = new ChangeLog()
{
Id = default,
Collaborator

Не все базы так поймут. Лучше для новых сущностей их генерировать самостоятельно (в идеале по седьмой версии).

Не все базы так поймут. Лучше для новых сущностей их генерировать самостоятельно (в идеале по [седьмой версии](https://steven-giesel.com/blogPost/ea42a518-4d8b-4e08-8f73-e542bdd3b983)).
@ -0,0 +279,4 @@
.Select(group => new
{
Min = group.Min(e => e.Creation),
Max = group.Max(e => e.Creation),
Collaborator

Не учитывает даты устаревания

Не учитывает даты устаревания
on.nemtina added 1 commit 2024-12-05 18:14:05 +05:00
on.nemtina added 1 commit 2024-12-06 14:55:23 +05:00
on.nemtina added 1 commit 2024-12-06 16:19:56 +05:00
on.nemtina added 1 commit 2024-12-06 16:34:45 +05:00
on.nemtina added 1 commit 2024-12-06 17:06:35 +05:00
on.nemtina added 1 commit 2024-12-09 10:15:29 +05:00
ng.frolov requested changes 2024-12-09 11:30:46 +05:00
Dismissed
@ -0,0 +101,4 @@
var result = 0;
using var transaction = await db.Database.BeginTransactionAsync(token);
try
Collaborator

try-catch тут и дальше не нужен. Rollback ролбэк будет вызван финализатором транзакции, если не был вызван комит

try-catch тут и дальше не нужен. Rollback ролбэк будет вызван финализатором транзакции, если не был вызван комит
on.nemtina added 1 commit 2024-12-09 14:45:44 +05:00
ng.frolov approved these changes 2024-12-09 15:00:40 +05:00
on.nemtina merged commit feb7e5d362 into master 2024-12-09 17:44:48 +05:00
on.nemtina deleted branch featute/ChangeLog 2024-12-09 17:44:48 +05:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: on.nemtina/persistence#6
No description provided.