iiuni / projektzapisy

System Zapisów na zajęcia w Instytucie Informatyki Uniwersytetu Wrocławskiego
https://zapisy.ii.uni.wroc.pl
32 stars 10 forks source link

Bug: Błędy typów w kodzie pomimo Typescripta #1438

Open MusicFreak456 opened 1 year ago

MusicFreak456 commented 1 year ago

Pracując przy komponentach Vue zauważyłem, że jest możliwość popełnienia w nich błędu typów pomimo użycia Typescripta. Np. w widgecie z powiadomieniami, na ten moment zlokalizowanym tutaj: link. Dało się (a nawet obecnie tak właśnie jest w kodzie produkcyjnym) utworzyć metodę deleteOne(i: number): Promise<void>, a następnie wywołać ją z argumentem typu string: deleteOne(elem.id), gdzie elem jest instancją klasy Notification, a id jej polem typu string.

Pawe16 commented 2 weeks ago

Problem wynika z użycia funkcji infer bilbioteki zod przy deklaracji typu Notification.

// Defines a notification scheme to validate and parse Notifications from JSON.
const notificationScheme = z
  .object({
    id: z.string(),
    description: z.string(),
    issued_on: z.string(),
    target: z.string(),
  })
  .transform((parsedObject) => {
    return {
      id: parsedObject.id,
      description: parsedObject.description,
      issuedOn: parsedObject.issued_on,
      target: parsedObject.target,
    };
  });
type Notification = z.infer<typeof notificationScheme>;

Typ wydedukowany w ten sposób nie zapewnia sprawdzania typów dla swoich pól - z tego powodu można przekazac argument typu string (w kodzie jest to elem.id) do funkcji przyjmujacej argumenty typu number. Jest to jedyne miejsce w kodzie calego projektu, gdy wykorzystywana jest funkcja infer. Po zamianie definicji typu na nastepująca:

type Notification = {
  id: string;
  description: string;
  issuedOn: string;
  target: string;
};

sprawdzanie typów działa ponownie dobrze, próba wywołania deleteOne(elem.id) nie powiedzie sie - zostaniemy poinformowanie o błedzie Argument of type 'string' is not assignable to parameter of type 'number'

lgpawel commented 1 week ago

(Śmieszna sprawa – kiedy błąd był zgłaszany, nie używaliśmy jeszcze zod, tylko spicery, czyżby błąd przetrwał przenosiny z jednej biblioteki na drugą?)

Typ wydedukowany w ten sposób nie zapewnia sprawdzania typów dla swoich pól - z tego powodu można przekazac argument typu string

Jak to się ma do faktu, że notificationScheme (też zresztą deklarowane przy użyciu funkcji bibliotecznych zod) wygląda na ręcznie zadeklarowane jako składające się z czterech stringów? Być może te dwie rzeczy są od siebie niezależne, ale jednak trochę wygląda na to, że jedna może wpływać na drugą.

Zaproponowana zmiana w kodzie zawiera zduplikowaną logikę i to oczywiście nie jest idealne. Czy pozostawienie infer (być może z jakąś mniej rozbudowaną modyfikacją) razem z wprowadzeniem wywołania parseInt nie będzie OK, skoro tak czy siak elem.id jest napisem? A jeśli nie, to czy możemy to jakoś inaczej zdeduplikować z powrotem, np. najpierw definiując Notification, a dopiero potem notificationScheme, ale bez czterokrotnego odwołania do z.string(), tylko do Notification właśnie?

Pawe16 commented 6 days ago

Z tego co się zorientowałem spicery i zod są dość podobnymi bibliotekami, dlatego wyjade mi się prawdopodobne, że błąd występuje w obu przypadkach. Natomiast jeśli chodzi o ręczną definicję typów w tym fragmencie kodu

.object({
    id: z.string(),
    description: z.string(),
    issued_on: z.string(),
    target: z.string(),
  })

, to jeśli dobrze rozumiem sposób w jaki działa zod, jest to tylko opis jakiego formatu danych oczekujemy, tzn np przy wywołaniu .then((r) => notificationSchemeArray.parse(r.data)) zostanie przeprowadzona walidacja czy r jest tablicą obiektów składających się z pól id, description, issued_on i target, wszystkich typu string. Zatem zdefiniowany ręcznie typ z.string() ma znaczenie, ale walidacja odbywa się dynamicznie, w trakcie trwania programu. Poza tym na już zwalidowanym obiekcie jest wykonywana transformacja

.transform((parsedObject) => {
    return {
      id: parsedObject.id,
      description: parsedObject.description,
      issuedOn: parsedObject.issued_on,
      target: parsedObject.target,
    };

, i tu typy już nie są zadeklarowane. Wydawałoby się naturalne, gdyby pola obiektu po transformacji również były typu string, ale wygląda na to że są typu any i podjęte przeze mnie próby jawnej definicji typów np:

    return {
      id: parsedObject.id as string,
      description: parsedObject.description as string,
      issuedOn: parsedObject.issued_on as string,
      target: parsedObject.target as string,
    };

nie rozwiązały problemu. Wydaje mi się, że typy są więc sprawdzane tylko dynamicznie. Jeśli chodzi o uniknięcie duplikacji kodu, to moim zdaniem zostawienie funkcji infer i wywołanie funkcji deleteOne z parametrem parseInt(elem.id) będzie dobrym rozwiązaniem. Nie rozwiązuje to stricte problemu tego issue, bo Typescript wciąż nie będzie informował nas o przypadkach, gdy takiego parseInta ktoś zapomniałby napisać, ale w tym przypadku (a jest to jedyny taki przypadek w kodzie) nie wydaje się to niebezpieczne.