[MSSQL] Sprawdzanie parametrów procedur

Tworzę pewien system i piszę procedury składowane. I mam pytanie takie - czy jest sens sprawdzania w takiej procedurze prawidłowości jej parametrów? Wszakże i tak baza nie pozwoli na wstawienie niepoprawnych wartości naruszających klucze bądź ograniczenia, ale w ten sposób mogę zwrócić własny kod błędu i komunikat oraz wyświetlić go użytkownikowi.
Jakie są best practices w tej kwestii?

Prosty przykład:

CREATE PROCEDURE [dbo].[ProducenciDodaj]
	@nazwa				varchar(50),
	@komunikat 			nvarchar(200) output
AS
BEGIN

	-- sprawdzenie, czy istnieje już producent o podanej nazwie
	IF EXISTS (SELECT id_producenta FROM Producenci WHERE nazwa = @nazwa)
	BEGIN
		SET @komunikat = 'Błąd dodawania! W bazie już istnieje producent o nazwie: "' + @nazwa + '"'
		RETURN -2
	END

	-- sprawdzenie, czy podana nazwa nie jest pusta
	IF LEN(@nazwa) = 0
	BEGIN
		SET @komunikat = 'Błąd dodawania! Nazwa producenta nie może być pusta.'
		RETURN -4
	END

	-- dodanie nowego rekordu
	INSERT INTO Producenci (nazwa) 
	VALUES (@nazwa) 
	-- informacje zwrotne
	SET @komunikat = 'Dodano producenta o nazwie: "' + @nazwa + '"'
	RETURN 0

END

Moim zdaniem takie sprawdzenie może być ok.
Tylko najpierw sprawdzaj, czy przekazany parametr nie jest pusty, a dopiero potem, czy istnieje taki wpis :)

Ja bym to ukrócił: zamiast osobnych procedur Add i Set, dał AddSet w taki sposób:

if exists(select Id from Table where Id=@Id)
update Tabela set (

)
else insert into Tabela values (


)

Baza danych jest od danych, a nie od walidacji ..

Dzięki za uwagi :)

Deti napisał(a)

Baza danych jest od danych, a nie od walidacji ..

To chyba przesada, wówczas wszelkie klucze i ograniczenia byłyby zbędne.

Klucze i ograniczenia sa do pilnowania i gwarancji spojnosci, nie do walidacji danych. IMO walidacja jest ok, ale lepiej chyba przeprowadzic ja na warstwie wyzszej, chocby i logiki biznesowej albo i wczesniej jesli sie da. Tym sposobem latwiej bedziesz mogl wprowadzac zmiany w tej walidacji.

inna sprawa, że po ch** wysyłąć do bazy x bajtów danych, tylko po to, żeby się dowiedzieć (co też po sieci musi wrócić), że jeden bajt jest zły i całą operację trzeba powtórzyć.

Generalnie baza ma przechowywać i zwracać żądane dane, pilnując przy okazji ich spujności i nie "zastanawiać" się dodatkowo co to za dane są - to ma robić klient

Jasne, że mam zamiar sprawdzać poprawność wprowadzanych danych na poziomie interfejsu.
Chciałem po prostu zrobić dodatkowy poziom zabezpieczeń na wypadek, gdyby np. kiedyś został zbudowany nowy interfejs do tej bazy. Ale jak rozumiem, Waszym zdaniem to bez sensu.

@johny_bravo - a ograniczenia CHECK, które pozwalają sprawdzić np. czy wpisywany do kolumny tekst jest pusty albo czy wpisywana data nie jest późniejsza niż aktualna, nie są formą walidacji?

Wiesz, wszystko moze byc uznane za forme walidacji, nawet ograniczenie NOT NULL czy to, ze pole jest typu datetime ;) Tyle, ze te ograniczenia na bazie to zwykle pilnowanie spojnosci i raczej tak powinny byc traktowane. Klasyczna walidacja, ktora opiera sie na zalozeniach logiki programu jest jednak w wyzszych warstwach.

Jeszcze cos na temat ograniczen w bazie. One sluza do utrzymania danych w ryzach, zeby nie rozjechaly sie relacje i zaleznosci logiczne miedzy danymi. Natomiast interpretacja tych danych, pelna i zdecydowanie glebsza musi byc w warstwie wyzszej, bo to ktoras z tych warstw bedzie umeic je w pelni zinterpretowac. Mozna pokusic sie o przyklad walidacji np. PESELu w bazie. Format sie 'raczej' nie zmienia i niby mozna sobie funkcyjke napisac, ktora go sprawdzi. Rzecz w tym, ze nawet jesli format sie nie zmieni, to moga sie zmienic inne rzeczy, ktore spowoduja, ze trzeba bedzie wprowadzic poprawke w ograniczeniu, nastepnie kolejna poprawke i jeszcze jedna. Te same poprawki trzeba bedzie dac w normalnej walidacji interfejsu. Juz zaczyna sie mieszac i juz zaczynamy sie powtarzac. A to wrecz gwarancja podwojnej ilosci bledow. I tak dalej...

PS. Jakby ktos pytal co sie moze z PESELem zmienic, to np. mozemy nagle zaczac obslugiwac obcokrajowcow z innymi numerami lub w ogole bez nich ;)

No dobra. A załóżmy, że chcę przechowywać w bazie wartość liczbową, np. masę chorego w kg. Nie może być ona NULL, bo każdy przyjmowany pacjent waży, i takie daję ograniczenie. Do przechowywania używam typu smallint (wszak tinyint dla niektórych to za mało). No i wiadomo, że masa nie może być ujemna.
Czy jest zatem sens dawać w bazie CHECK na kolumnie z masą i sprawdzanie czy wprowadzana wartość jest ujemna, czy nie?

Wedlug mnie to troche 2 rzeczy pisac walidacje w procedurze a korzystac z wbudowanych mechanizmow. Po pierwsze check jest pod reka i uzycie go to praktycznie zero ryzyka bledu i 2 sekundy. Po drugie takie warunki sa praktycznie zawsze stale - masa nigdy nie powinna byc ujemna, niewazne jakie wymagania sie zmieniaja.

Co innego dopisywac takie warunku programistycznie w procedurze. Musisz to napisac, musisz sie przy tym nie pomylic, musisz o tym pamietac, kiedy cos sie bedzie zmieniac.

Ja bym takie warunki ustalal tak jak powiedzialem - jesli sa oczywiste i uwarunkowania chronia przed oczywistym bledem. Z tego samego powodu (miedzy innymi) dajemy rozne typy kolumn zamiast wszedzie dawac stringa i parsowac sobie gdzies narazajac sie, ze przyjdzie 'cokolwiek'.

Czyli dam CHECK tam, gdzie trzeba, a procedury uproszczę do pojedynczych operacji.

Dzięki za pomoc :)

Misiekd napisał(a)

inna sprawa, że po ch** wysyłąć do bazy x bajtów danych, tylko po to, żeby się dowiedzieć (co też po sieci musi wrócić), że jeden bajt jest zły i całą operację trzeba powtórzyć.

No to racja.
Ale jak w interfejsie sprawdzić, czy np. wpisana przez użytkownika nowa nazwa producenta nie istnieje jeszcze w bazie (a w bazie kolumna nazwa jest Unique)? Chyba nie mam innego wyjścia, niż wysłać dane do procedury, a jeśli ona zwróci -4, to przekazać użytkownikowi info o błędzie.

Mozna po prostu o nia zapytac niz uruchamiac caly mechanizm np. dodawania. Zauwaz, ze operacje w procedurach zwykle nie sa atomowe. To moze byc np. dodanie 3 wierszy, aktualizacja 3 nastepnych i dodanie na ich podstawie 3 kolejnych. Wszystko w transakcji z obsluga bledow. Uruchamianie czegos takiego to kombajn w porownaniu do odpytania czy istnieje wiersz o jakiejs tam wartosci.

Czyli scenariusz wygląda tak:

1. Użytkownik wpisuje nową nazwę i żąda jej dodania.
2. Interfejs pyta bazę, czy taka nazwa już istnieje, baza informuje, że nie istnieje.
3. Ponieważ nie istnieje, to jest wywoływana procedura dodająca.

Ale jeśli między punktem 2 a 3 jakiś inny użytkownik zdąży wstawić tę nazwę, to i tak procedura dodająca nie wykona się poprawnie.

Więc co jest bardziej opłacalne?

1. Wysłać x bajtów do bazy, po to, żeby sprawdzić czy coś w niej istnieje, a następnie wysłać y bajtów w celu dodania nowego wpisu - co nie zawsze musi zadziałać.
2. Wysłać y bajtów w celu dodania nowego wpisu, a jeśli baza na to nie pozwala, to nie zostanie to po prostu wykonane - ale jest to tylko druga część operacji z pkt 1).

A co do atomowości - to u mnie i tak większość procedur to właśnie pojedyncze akcje.

Punkt 2 i 3 mozna objac transakcja, bo nie ma miedzy nimi interakcji z uzytkownikiem.

IMO lepiej jest zawrzec tego 'ifa' w kodzie, czyli warstwie wyzszej, bo to ona odpowiada za logike. To tak logika definiuje czy mozna dodac uzytkownika o takiej samej nazwie, czy nie. Oczywiscie mozna powiedziec, ze czesc tej logiki i tak przechodzi na baze (bo unique constraint), ale to jest w celu gwarancji spojnosci, a nie sterowania dzialaniem aplikacji.

Takie dzialanie, ze wywolujemy cos 'dalekiego' (bo sieganie do bazy nie jest czyms lekkim wewnatrz kodu) i wrecz spodziewamy sie bledu kojarzy mi sie ze sterowaniem przebiegiem danego procesu w aplikacji za pomoca wyjatkow. Czekasz na wyjatek i za jego pomoca decydujesz o dalszym postepowaniu w duzej ilosci przypadkow, nie w sytuacjach wyjatkowych. To nie jest to do czego sluza wyjatki, ani do czego sluzy baza danych.

Bledy zwracane z bazy danych z zalozenia maja byc wyjatkowe. Np. naruszenie klucza obcego, wstawienie nulla w not null - to sa sytuacje, ktore nie naleza do normalnego zachowania aplikacji. Kolizje nazw nie zawieraja sie w ten konwencji. Oprocz tego, ze w tym wypadku dorzucasz do bazy interpretacje danych - nazwa uzytkownika ma sens z punktu widzenia logiki aplikacji, baza nie ma pojecia co to. I nie powinna miec.

Zwroc tez uwage, ze im glebiej warstwa lezy tym mniej zmian powinno byc na niej wykonywanych. Zmiany takie powoduja zwykle lawinowe zmiany w warstwach wyzszych, wiec to zalozenia to dmuchanie na zimne :) Ladowanie logiki wglab, czyli do warstwy przechowywania danych, od ktorej zwykle cala reszta mocno zalezy naraza Cie na zmiany w wyniku zmian w wymaganiach klienta. Pomysl co sie stanie jesli nagle klient zazyczy sobie, ze nazwa nie bedzie unikalna. W normalnym przypadku (tylko unique i dodatkowe zapytanie sprawdzajace) usuwasz ograniczenie unique i komentujesz ifa. Koniec.
W drugim przypadku zmieniasz procedure oraz komentujesz/zmieniasz spora czesc kodu odpowiedzialna za sprawdzenie kodu wracajacego z bazy i obsluge tej sytuacji. Tutaj moze nie widac tej zmiany mocno, bo to tylko mala zmiana, ale w przypadku kilku takich zmian w paru miejscach + nagromadzenie kilku warstw w celu abstrakcji - lezysz.

//edit - sie rozpisalem ;)

Punkt 2 i 3 mozna objac transakcja, bo nie ma miedzy nimi interakcji z uzytkownikiem.

Zupełnie nie rozumiem w jaki sposób?

Robię aplikację MVC w ASP.NET. Dostęp do danych realizowany mam przy użyciu procedur składowanych. Jak już ustaliliśmy wcześniej w wątku procedury nie sprawdzają danych, nie zwracają komunikatów ani moich własnych kodów błędów - od tego są CHECK na kolumnach tabel. Procedury są teraz maksymalnie uproszczone w porównaniu z moim pierwotnym projektem.
Zatem:

1. Gdy sprawdzam czy producent o danej nazwie istnieje w bazie wywołuję w kodzie Controllera procedurę.
2. Gdy dodaję nowego producenta to wywołuję procedurę.

Bledy zwracane z bazy danych z zalozenia maja byc wyjatkowe.

SQL Server przy wykonywaniu procedur "sam z siebie" zwraca:
0 dla prawidłowego wykonania
-4 dla naruszenia Unique
-6 dla naruszenia Null i Check

Ja to przechwytuję w kodzie aplikacji (wynik działania procedury to nie wyjątek przecież) i w zależności od tego informuję użytkownika o powodzeniu / niepowodzeniu operacji. Ponadto waliduję dane pobrane od użytkownika. Jestem w stanie sprawdzić, czy nie podał pustego znaków albo np. niewłaściwych znaków. Nie wiem tylko jak na poziomie interfejsu sprawdzić, czy podał unikalną wartość. Bo teoretycznie jest szansa, że między odwołaniem do bazy w celu sprawdzenia a odwołaniem w celu wstawienia danych ktoś już coś takiego wstawi.

Kurde, chyba rozdmuchuję problem, który tak naprawdę nie istnieje :)

odnośnie

Ale jak w interfejsie sprawdzić, czy np. wpisana przez użytkownika nowa nazwa producenta nie istnieje jeszcze w bazie (a w bazie kolumna nazwa jest Unique)?

hola, hola. Sprawdzanie, czy waga dodawanego ludka jest > 0, czy pesel jest prawidłowy, i innych takich to jest całkiem coś innego niż błąd not unique!

Więc może przestańmy mieszać, bo to o co pytałeś na początku da się sprawdzić w aplikacji BEZ WYSYŁANIA jakich kolwiek danych do bazy, a tego o co pytasz teraz nie da się sprawdzić w aplikacji bez wysyłania zapytania do bazy. Jeśli chodzi o unique np. to jeśli procent sytuacji, kiedy user rzeczywiście istnieje (i jest błąd) do tych, kiedy nie istnieje nie przekracza 33% to nie ma co sobie zawracać gitary wcześniejszym pytanie o to czy user jest czy nie.

//edit
podsumowując

1. sprawdzanie peselu, nipu, wzrostu, zarostu, wagi i co Ci tam przyjdzie na myśl, ale co nie ma związku z samym przechowywaniem danych sprawdzasz w warstwie prezentacji (pozwala Ci to np. po wyjściu z edita, gdzie wpisano nieprawidłową wartość natychmiast poinformować o tym fakcie usera). Tutaj też bardzo często sprawdza się ograniczenia not null i FK (przy insert i update) (generalnie jeśli chodzi o FK to po prostu tak konstruuje się formularz, żeby nie było możliwe wpisanie do pola FK czegokolwiek poza tym, co się tam może znaleźć).
2. po stronie bazy zostaje sprawdzanie unique i FK przy delete. Tutaj dodatkowo jeszcze dochodzą sytuacje specjalne, np. mamy w bazie procedurę, która zapisuje, np. z tabel tymczasowych, dokument sprzedaży. Sama aplikacja wcale nie musi wiedzieć, czy wszystkie towary na dokumencie mają przypisaną cenę, czy po różnych upustach ceny te nie przekroczą jakiegoś minimum itp. Wyliczane to jest dopiero w tej procedurze i jeśli nie można zapisać tego dokumentu to trzeba rzucić wyjątkiem

somekind napisał(a)

Punkt 2 i 3 mozna objac transakcja, bo nie ma miedzy nimi interakcji z uzytkownikiem.

Zupełnie nie rozumiem w jaki sposób?

No ja to widze tak:

nowa_transakcja();
if(sprawdz_selectem_istnienie_nazwy() == nie_istnieje)
{
  wykonaj_procedure();
  result = ok;
}
else
  result =  uzytkownik_istnieje;
koniec_transakcji();
return result;

Dzieki temu zaraz po wywolaniu selecta masz potencjalne wywolanie procedury, a transakcja gwarantuje, ze w tzw. miedzyczasie sie nic nie zmieni.

Ja to przechwytuję w kodzie aplikacji (wynik działania procedury to nie wyjątek przecież)

Sytuacja naruszenia spojnosci bazy ma byc wyjatkowa. Od tego masz walidacje danych na wyzszej warstwie, zeby 95% danych wyslanych do bazy bylo poprawnych. Pozostale 5% to sytuacje wlasnie wyjatkowe, rzadkie bledy, itp.

Bo teoretycznie jest szansa, że między odwołaniem do bazy w celu sprawdzenia a odwołaniem w celu wstawienia danych ktoś już coś takiego wstawi.

Od tego sa wlasnie transakcje.

Kurde, chyba rozdmuchuję problem, który tak naprawdę nie istnieje :)

Troche tak. Za bardzo kombinujesz, zamiast rozdzielic role tak jak powinny byc rozdzielone. Zostaw bazie przechowywanie danych w poprawnej formie, bo do tego zostala stworzona. Mozna oczywiscie i pol aplikacji napisac w procedurach, ale nie od tego sa i tak naprawde tylko utrudnisz sobie zycie.

johny transakcje są troszkę do czegoś innego i akurat w Twoim przypadku to nie przejdzie

@up: No w sumie... Rozpedzilem sie troche ;)

Misiekd napisał(a)

Więc może przestańmy mieszać, bo to o co pytałeś na początku da się sprawdzić w aplikacji BEZ WYSYŁANIA jakich kolwiek danych do bazy, a tego o co pytasz teraz nie da się sprawdzić w aplikacji bez wysyłania zapytania do bazy. Jeśli chodzi o unique np. to jeśli procent sytuacji, kiedy user rzeczywiście istnieje (i jest błąd) do tych, kiedy nie istnieje nie przekracza 33% to nie ma co sobie zawracać gitary wcześniejszym pytanie o to czy user jest czy nie.

Ok, to właśnie chciałem wiedzieć, bo nie bardzo rozumiałem o co chodziło Johnemu :)

Misiekd napisał(a)

1. sprawdzanie peselu, nipu, wzrostu, zarostu, wagi i co Ci tam przyjdzie na myśl, ale co nie ma związku z samym przechowywaniem danych sprawdzasz w warstwie prezentacji (pozwala Ci to np. po wyjściu z edita, gdzie wpisano nieprawidłową wartość natychmiast poinformować o tym fakcie usera). Tutaj też bardzo często sprawdza się ograniczenia not null i FK (przy insert i update) (generalnie jeśli chodzi o FK to po prostu tak konstruuje się formularz, żeby nie było możliwe wpisanie do pola FK czegokolwiek poza tym, co się tam może znaleźć).

Sprawdzenie w interfejsie to nie jest dla mnie żaden problem, to akurat wiem jak rozwiązać.

Ale jeszcze jednej rzeczy nie wiem. Jak rozumiem, to nie ma sensu dawać ograniczeń CHECK na kolumnach bazy. To po co one w takim razie są?

johny_bravo napisał(a)

No ja to widze tak:

nowa_transakcja();
if(sprawdz_selectem_istnienie_nazwy() == nie_istnieje)
{
  wykonaj_procedure();
  result = ok;
}
else
  result =  uzytkownik_istnieje;
koniec_transakcji();
return result;

Dzieki temu zaraz po wywolaniu selecta masz potencjalne wywolanie procedury, a transakcja gwarantuje, ze w tzw. miedzyczasie sie nic nie zmieni.

Ale implementacyjnie to miałoby być coś w stylu SqlConnection.BeginTransaction()? Bo w sumie nadal nie kapuję.

sprawdzanie wartości parametrów w procedurach czy na polach robi się np. wtedy, jeśli jest to część udostępniona "światu" jeśli w taki sposób zrobisz import z obcych systemów

Ok, wielkie dzięki za pomoc.