S>Факт добавления в trackable entities Id не даст, Id даст бд. Без await в Id будет какой-нибудь 0
это понятно, зачем await при добавлении в in-memory, надо понять, знает ли человек что savechanges также осуществляет адиз бак пропагейшн

был отачсти неправ, AddAsync() начинает трачить previously untracked entity.

This method is async only to allow special value generators, such as the one used by 'Microsoft.EntityFrameworkCore.Metadata.SqlServerValueGenerationStrategy.SequenceHiLo', to access the database asynchronously. For all other cases the non async method should be used.

尿Ǥ푙>был отачсти неправ, AddAsync() начинает трачить previously untracked entity.

尿Ǥ푙>This method is async only to allow special value generators, such as the one used by 'Microsoft.EntityFrameworkCore.Metadata.SqlServerValueGenerationStrategy.SequenceHiLo', to access the database asynchronously. For all other cases the non async method should be used.
尿Ǥ푙>


Т.е. AddASync таки лезет в базу, и именно так мне назначенный ID и прилетает до SaveChanges?

尿Ǥ푙>Мы обсуждаем вещи, сути которых не знаем.

В целом да, но видимо так поставлена задача у ТС

尿Ǥ푙> Обычно "hash" — это то что решает, нужно ли что то делать или нет на основании того если что то еще уже или нет. Если этот hash не идемпотентен, то возможно одна из ветвей логики, посвященная существующему хашу в приложении, никогда не будет выполняться, мне это кажется странным вот моя логика.

Здесь на мой взгляд понятно, что в БД кладутся не URL, а запросы, каждый со своим timestamp и соответственно ID (и hash).
И никакой идемпотентности тут не предполагалось: повторное обращение к методу приводит к новой записи в БД.
Чтобы не было путаницы, я бы предложил переименовать метод (SaveRequestToDb).

G>Т.е. AddASync таки лезет в базу, и именно так мне назначенный ID и прилетает до SaveChanges?

Лезет AddAsync в БД или нет зависит от того, как настроен DB-контекст.
В частности, от выбранной стратегии установки identity для сохраняемой entity.
Если используется генерация на уровне БД (например, IDENTITY column в MS SQL), то похода в БД не будет, но Id будет 0 до вызова SaveChanges.
Если используется SequenceHiLo или просто Sequence, то поход в БД может быть, а может не быть. Если, например, шаг sequence = 5, то первые пять вызовов пройдут без похода в БД, затем будет один поход для получения текущего значения sequence и следующие 5 вызовов снова будут без похода в БД.
Если же ID назначается пользовательским кодом (например это Guid или какой-то глобальный shared счетчик или какая-то еще кастомная стратегия), то похода в БД опять же не будет.

G>>Т.е. AddASync таки лезет в базу, и именно так мне назначенный ID и прилетает до SaveChanges?
RD>Лезет AddAsync в БД или нет зависит от того, как настроен DB-контекст.

Ко всему выше сказанному добавлю, что в условиях задачи (у топикстартера) нигде не сказано, что используется EF.
Это уже в комментариях предположили.
(я например, в разработке никогда с EF на практике не сталкивался, так что вообще не подумал бы в эту сторону )

M>Здесь на мой взгляд понятно, что в БД кладутся не URL, а запросы, каждый со своим timestamp и соответственно ID (и hash).
M>И никакой идемпотентности тут не предполагалось: повторное обращение к методу приводит к новой записи в БД.
M>Чтобы не было путаницы, я бы предложил переименовать метод (SaveRequestToDb).

мне ничего не понятно. в рестуфул сервисах вы много информации из Request Url — выудите кроме как для гетов да и то не всегда? Если это аудит траил — правильнее использовать fire&forget. Мы гадаем на кофейной гуще, друг мой.

Здравствуйте, RushDevion, Вы писали:

G>>Т.е. AddASync таки лезет в базу, и именно так мне назначенный ID и прилетает до SaveChanges?

RD>Лезет AddAsync в БД или нет зависит от того, как настроен DB-контекст.
RD>В частности, от выбранной стратегии установки identity для сохраняемой entity.
RD>Если используется генерация на уровне БД (например, IDENTITY column в MS SQL), то похода в БД не будет, но Id будет 0 до вызова SaveChanges.
RD>Если используется SequenceHiLo или просто Sequence, то поход в БД может быть, а может не быть. Если, например, шаг sequence = 5, то первые пять вызовов пройдут без похода в БД, затем будет один поход для получения текущего значения sequence и следующие 5 вызовов снова будут без похода в БД.
RD>Если же ID назначается пользовательским кодом (например это Guid или какой-то глобальный shared счетчик или какая-то еще кастомная стратегия), то похода в БД опять же не будет.

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

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>Как я сказал ранее, скорее всего первый await не нужен, достаточно простого добавлению в коллекцию trackable entities. Два раза базу дергать не надо.
Скорее всего, не нужен второй.
Я бы на месте пользователя этого кода крайне бы удивился тому, что у меня метод по генерации хеша для запроса вдруг закоммитит транзакцию в базу.

Здравствуйте, Glestwid, Вы писали:

G>Вот, на интервью скинули такой метод на ревью. Что в нем можно улучшить и(или) ускорить, помимо добавления логирования ошибок ?

	Код спрятал
	G> public async Task<string> ToHash(CreateHashCommand request, CancellationToken cancellationToken) G> { G> try G> { G> if (string.IsNullOrWhiteSpace(request.Url)) G> throw new InvalidEnumArgumentException(); G> var urlInDb = new Url() G> { G> Created = DateTime.UtcNow, G> CreatedBy = "", G> OriginalUrl = request.Url G> }; G> await _context.Urls.AddAsync(urlInDb, cancellationToken); G> await _context.SaveChangesAsync(cancellationToken); G> var urlHash = _hashgen.EncodeLong(urlInDb.Id); G> return urlHash; G> } G> catch (Exception ex) G> { G> throw; G> } G> }

В целом о ревью. Коллеги выше предлагают чуть ли не предложить всё переделать. Это, несомненно, возможно, но не интересно. На "ревью" во время собеседования, когда вам, зачастую, не говорят, что же хотелось сделать и, учитывая, что вы, скорее всего не знаете, как и что организовано там, откуда этот код, мне кажется полезнее не делать безосновательных догадок о том, чего вы не знаете и больше опираться на то, что есть в коде. Например, в коде есть сохранение контекста после добавления строки. Значит, скорее всего, так и нужно. Нет логирования. Значит, скорее всего так и нужно: возможно, логирование ошибок есть где-то выше, а логирование запросов и параметров, наоборот, ниже). Если у вас есть, допустим, код всего класса и в других похожих методах логирование сделано вот так вот, в try/catch-е на всё тело тогда есть о чём поговорить, но вот по одному методу судить о нужности логирования именно тут чего-то мне кажется преждевременно.

Зато тут вот есть пустой catch. Вот такое вот совершенно точно бесмысленно и если его убрать, то станет намного лучше без потери вообще чего бы то ни было.

И по мелочи:

• …, CancellationToken cancellationToken = default) — так обычно удобнее объявлять CancellationToken-параметры.
  
• Если CreateHashCommand это ссылочный тип, то аргумент не мешало бы проверить на null.
  
• Вместо InvalidEnumArgumentException уместнее бросать ArgumentException или ArgumentNullException, как это делается, например, здесь.
  
• С сущностью (Url). Я б спросил, а это так и нужно, что CreatedBy = "" или это ошибка? Если это ошибка, то что там должно быть? Если это не ошибка и так и нужно, чтобы ORM сам подставил это поле нужное значение, возможно, ORM сам мог бы подставлять и Created?
  
• _context мне кажется лучше заводить локальный, раз уж мы его сохраняем здесь же.
  
• Подумать и решить, не нужно ли нам добавить .ConfigureAwait(continueOnCapturedContext: false). Скорее всего, нужно.

Совсем не ясны пляски с _hashgen.EncodeLong(urlInDb.Id) Это такая попытка замаскировать от пользвателя сгенерированный идентификатор?

尿Ǥ푙>абсолютно верно. теоретически это может быть нужно и параллелить это вряд ли можно.

А параллелить при всем желании не получится.
DbContext (если речь про EF Core) не потокобезопасен.
Если память не изменяет, там InvalidOperationException стреляет, когда обнаружен параллельный доступ из нескольких потоков.

такова суть EF — SaveChanges() коммитит все неизмененное для контекста, жалобы — к майкрософту выяснилось, что первый awaitable инициирует тракинг, и асинхронность нужна ему тех стратегий генерации ключа, что требуют запроса к базе хайло и все такое.

к SaveChanges() столько же претензий сколько и плюсов, но это это конвенциональных способ сохранения измений в базу.

G>Т.е. AddASync таки лезет в базу, и именно так мне назначенный ID и прилетает до SaveChanges?

он лезет в базу но не за айди. указания на действие на тот момент просто нет, ему нужны данные для стратегий генерации ключа основанных на данных.

M>Ко всему выше сказанному добавлю, что в условиях задачи (у топикстартера) нигде не сказано, что используется EF.
M>Это уже в комментариях предположили.
M>(я например, в разработке никогда с EF на практике не сталкивался, так что вообще не подумал бы в эту сторону )

основываясь на коде мы можем предположить это с высокой степенью вероятности

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:
尿Ǥ푙>к SaveChanges() столько же претензий сколько и плюсов, но это это конвенциональных способ сохранения измений в базу.
Это я как раз понимаю. Претензии не к SaveChanges, а к методу, который слишком много себе позволяет.
Он явно служебный, вызывается в рамках какого-то контроллера. Вот тот и должен решать, когда сделать SaveChangesAsync(), а не какой-то там вычислитель хеша. Может там кто-то потом отмену изменений запросит.

Здравствуйте, Sinclair, Вы писали:

S>Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:
尿Ǥ푙>>к SaveChanges() столько же претензий сколько и плюсов, но это это конвенциональных способ сохранения измений в базу.
S>Это я как раз понимаю. Претензии не к SaveChanges, а к методу, который слишком много себе позволяет.
S>Он явно служебный, вызывается в рамках какого-то контроллера. Вот тот и должен решать, когда сделать SaveChangesAsync(), а не какой-то там вычислитель хеша. Может там кто-то потом отмену изменений запросит.

такое же ощущение, кусок чего то бОльшего. Я бы оставил как есть и попросил бы человека покрыть этот код юнит тестами, думаю в сл. раз был бы и репозиторий и интерфейс работы с базой более строгий.

_FR>Совсем не ясны пляски с _hashgen.EncodeLong(urlInDb.Id) Это такая попытка замаскировать от пользвателя сгенерированный идентификатор?

Нет, это пример сервиса short url. Там юзеру выдают хеш реального урла.

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>это такая игра/процесс, лучше пройти ее на берегу для обеих сторон. Предложение дефолтить значния в конструкторе немного "странные", а с датами так и прсто некорректные.

??? o_O ???

Почему они некорректные?
Вариант А:

class Url
{
  public Url()
  {
  }
}

...

var url = new Url{OriginalUrl = foo, Created = DateTime.Now);  //Повторяется 5 раз в разных местах

Вариант Б:

class Url
{
  public Url(string url)
  {
    Created = DateTime.Now;
    OriginalUrl = url;
  }
}

...

var url = new Url{foo);  //Повторяется 5 раз в разных местах

Второй вариант банально проще (меньше повторяющегося кода) и чище (разделено "что" и "как"). Ну и если URL добавляется ассоциативную таблицу, то релевантные поля должны быть readonly.

вообще шутил, но:

1. Даты могут привязаны к бизнес процессу а не ко времени создания объекта, а могут — нет.
2. Вы можете быть удивлены, но юзер в современных сервисах часто представлен токеном неизвестным (и не должным быть известным) на момент создания моделей. Если по-простому, для работы с секьюрити объектами наши бизнесс объекты должны иметь exposure на объекты секьюрити, что не есть хорошо. Это, конечно, больше вопрос дизайна, но это тоже надо учитывать. Сценарии типа — выполнить от лица и так далее, да много чего еще будут имет более простую реализацию если мы придерживаемся простого принципа — объекты отдельно, секьюрити — отдельно. Мой тейк таков — если уж и иметь секьюрити частью домена в качестве "карго компромиса", то по крайней мере не давать домену управлять ими.

Здравствуйте, RushDevion, Вы писали:

RD>Если используется SequenceHiLo или просто Sequence, то поход в БД может быть, а может не быть. Если, например, шаг sequence = 5, то первые пять вызовов пройдут без похода в БД, затем будет один поход для получения текущего значения sequence и следующие 5 вызовов снова будут без похода в БД.

Такой вопрос, а для кого эти стратегии расчитаны? Т.е. это должен быть один экземпляр приложения,
который работает с бд. Как множество микросервисов с SequenceHiLo не пересекутся тогда?
Т.е. в рамках одного процесса все это можно контролировать, а если процессов больше?
Или там при создании db контекста, он(контекст) лезет в базу (один раз как минимум) и резервируют
себе соотв. HiLo? .

Если короче-- каковы сценарии hilo?