Dużo zapytań do bazy przy walidacji

0

Mam aplikację, która umożliwia użytkownikom łączenie się grupy. Przynależności są definiowane przez rekordy tabeli GroupMemberships zawierającą klucze obce GroupId i UserId składające się jednocześnie na composite key. Przy dodawaniu użytkownika do grupy trzeba wykonać zapytania do tabeli GroupMemberships, aby sprawdzić, czy dodawany użytkownik już nie jest członkiem tej grupy, do tabeli Users aby sprawdzić, czy użytkownik istnieje, to tabeli Groups, aby sprawdzić, czy grupa istnieje i czy dodający użytkownik jest jej adminem, i w końcu do GroupMemberships. Tylko że to są aż cztery zapytania do tak prostej czynności. Zastanawiam się, czy nie zostawić jedynie walidacji tego, czy użytkownik jest adminem. Pozostałe przypadki, przed którymi próbuję się chronić, są mało prawdopodobne i nie powodują luk bezpieczeństwa, bo przecież SQL nie pozwoli na zapis i serwer zwróci Internal Server Error. Co sądzicie?

public async Task<Result> Handle(AddMemberToGroupCommand command, CancellationToken cancellationToken)
        {
            var groupMembershipTask = dbContext.GroupMemberships.AnyAsync(
                gm => gm.GroupId == command.GroupId && gm.UserId == command.UserId, cancellationToken);

            var userTask = dbContext.Users.AnyAsync(u => u.Id == command.UserId, cancellationToken);

            var groupTask = dbContext.Groups.FindAsync(command.GroupId);

            var isUserMember = await groupMembershipTask;
            var isUserExisting = await userTask;
            var group = await groupTask;
            
            if (isUserMember)
            {
                return new Result(ErrorName.NotValid, "Użytkownik jest już członkiem tej grupy.");
            }

            if (!isUserExisting)
            {
                return new Result(ErrorName.NotValid, "Użytkownik o podanym id nie istnieje.");
            }

            if (group == null)
            {
                return new Result(ErrorName.NotValid, "Grupa o podanym id nie istnieje.");
            }

            if (group.CreatedById != authContext.GetCurrentUserId())
            {
                return new Result(ErrorName.NotAuthorized, "Nie możesz dodawać członków do nieswojej grupy.");
            }

            var membership = new GroupMembership
            {
                GroupId = command.GroupId,
                UserId = command.UserId
            };

            dbContext.GroupMemberships.Add(membership);
            await dbContext.SaveChangesAsync(cancellationToken);

            return Result.SuccessfulResult;
        }
3

Nie wiem czy obcinanie logiki biznesowej to dobry pomysł na optymalizacje, chociaż na pewno można byłoby dużo ugrać na wydajności zakomentowując 70% kodu całej aplikacji.

Mam aplikację, która umożliwia użytkownikom łączenie się grupy.

I jak często użytkownicy by wykonywali tą operację? raz na 10ms / 100ms / 1sec / 10sec / 1min / 1h / 1day / 1 week / 1 month?

Jeżeli te cztery zapytania są rzadziej niż 30sec (skaluj wraz z liczbą użytkowników) to raczej nie ma co się przejmować.

Ale ok, lecimy bez sprawdzania.

Robisz coś na nullu - ile będzie Cię kosztować wyjątek + redirect do error page? czy może jednak wszystko w try catch aby wrócić do poprzedniej strony i zwrócić Exception Message, co samo w sobie jest słabym pomysłem, no ale jaki inny błąd zwrócisz userowi bez walidacji?

Jak tobie z perspektywy użytkownika użytkowałoby się aplikację, która po wysłaniu np. formularza zwraca błąd typu coś źle wpisałeś, ale dowiedz się sam :P

isUserExisting

UserExists?

1

A nie mógłbyś mapować błędów rzucanych przez serwer bazy danych na user-friendly komunikaty, zamiast wycinać walidację albo strzelać zapytaniami jak z karabinu?

Nie wiem, na ile szczegółowe / ogólne są w .NET błędy rzucane przez naruszenia, ale powinieneś być w stanie to jakoś zmapować ;)

EDIT pogóglaj bo nie zdziwię się, jeśli na takie podstawowe błędy naruszeń są już gotowe mappery, być może w ramach samego EF czy czego tam teraz C#-owy web używa. A nawet jeśli nie, to obsłużenie duplikatów / nieistniejących kluczy itp ręczną rzeźbą nie powinno być zbyt skomplikowane ;)

1
WeiXiao napisał(a):

Robisz coś na nullu - ile będzie Cię kosztować wyjątek + redirect do error page? czy może jednak wszystko w try catch aby wrócić do poprzedniej strony i zwrócić Exception Message, co samo w sobie jest słabym pomysłem, no ale jaki inny błąd zwrócisz userowi bez walidacji?

W którym miejscu robię coś na nullu?

Jak tobie z perspektywy użytkownika użytkowałoby się aplikację, która po wysłaniu np. formularza zwraca błąd typu coś źle wpisałeś, ale dowiedz się sam :P

Z tym raczej nie byłoby problemu, bo przecież użytkownik nie będzie wpisywał tych id ręcznie. ;p

isUserExisting

UserExists?

Ogólnie staram się nazywać zmienne logiczne w formie pytania. userExists to twierdzenie, a doesUserExist brzmi dla mnie jakoś dziwnie, chociaż co kto woli. ;p

1
nobody01 napisał(a):

Ogólnie staram się nazywać zmienne logiczne w formie pytania. userExists to twierdzenie, a doesUserExist brzmi dla mnie jakoś dziwnie, chociaż co kto woli. ;p

jak dla mnie userExists/doesUserExist brzmi 100 razy lepiej niż isUserExisting - czyli user teraz w sumie jest w trakcie egzystowania, ale za chwilę może sobie pójść na przerwę od egzystencji (po której wróci)? ;)

1

Tak jak napisał @WeiXiao nie powinieneś się tym przejmować. W części biznesowej znacznie ważniejsza jest ekspresywność i łatwość utrzymania kodu niż walka o ilość zapytań/milisekund. Dopiero jak się okaże że dany use case jest wolny lub bardzo obciąża serwer wtedy można by się nad tym pochylić, a tak szkoda czasu.

1

Ostatecznie chyba zostawię to tak jak jest. Nawet jeśli aplikacja zwolni, to napisanie takiego ubogiego exception handlera przechwytującego DbUpdateException i na podstawie kodu błędu w wewnętrznym SQLException rozpoznającego, że nastąpiło foreign key constraint violation, i zwracającego odpowiedni komunikat w zależności od nazwy tabeli nie jest takie trudne, skoro nawet mi się to udało. :p

@mad_penguin
Tu ktoś mający 153k reputacji pisze, że You can actually run parallel queries on a single context instance. The issue comes in with EF's change tracking and object fixup. These types of things don't support multiple operations happening at the same time, as they need to have a stable state to work from when doing their work. However, that really just limits your ability to do certain things. For example, if you were to run parallel saves/select queries, the results could be garbled. You might not get back things that are actually there now or change tracking could get messed up while it's attempt to create the necessary insert/update statements, etc. However, if you're doing non-atomic queries, such as selects on independent tables as you wish to do here, there's no real issue, especially, if you're not planning on doing further operations like edits on the entities you're selecting out, and just planning on returning them to a view or something
Więc chyba można ;)

0

Tak sobie teraz myślę, że przecież po dodaniu członka do grupy prawdopodobnie przekieruje się użytkownika to jakiejś strony z listą członków tej grupy, aby pokazać, że faktycznie jest nowy członek. Tylko że to oznacza ponowne wysłanie zapytania do GroupMemberships i pewnie też do Groups i Users, bo przecież przydałoby się wyświetlić nazwę tej grupy i nazwy użytkowników (może nawet z avatarami), którzy do tej grupy należą (nie wiem, jak to się robi w przypadku SPA - wysyła się ponowne zapytanie przy renderowaniu komponentu czy może jakoś na podstawie stanu aplikacji wydobywa się potrzebne dane?). Gdyby dołączyć jakąś bibliotekę dodająca cache drugiego poziomu dla EF Core, to można by zniwelować koszty dwóch zapytań przy walidacji (do GroupMemberships i Groups). Te zapytania trzeba by oczywiście zmodyfikować, aby móc ten cache wykorzystać. Dobrze myślę? ;)

1

Zastanów się, czy na pewno chcesz się pchać w raka pt. po spięciu usera z grupą zwróć grupę z userami - co jeśli będziesz miał use case pt. po spięciu usera z grupą zwróć usera z jego grupami? ;) będziesz trzymać dwie końcówki w API i każda będzie żenić usera z grupą, tylko zwróci co innego?

0

Właściwie to nie chciałem nic z tego endpointu zwracać - jedynie cachować potrzebne dane, ale to i tak nie miałoby sensu, bo zależnie od use case trzeba by cachować co innego - raz grupę z członkami, raz użytkownika z grupami. Zniszczyłeś cały mój genialny plan. ;(

1 użytkowników online, w tym zalogowanych: 0, gości: 1