REST API, Jeden kontroler wiele serwisów?

Hej,
Utknąłem w pewnych kwestiach przy przygotowywaniu prywatnego projektu (Dziennik szkolny), nie chciałbym na starcie pomieszać kilku rzeczy żeby pod koniec musieć wszystko sprzątać.

Nie do końca rozumiem pewnych kwestii.

1. Czy jeden kontroler może (czy jest to wskazane) korzystać z kilku serwisów?
   Mam StudentController, w którym chciałbym udostępniać listę oceń ucznia
   Pod adresem students/{id}/grades, do tego potrzebuję skorzystać z serwisu, który dostarczy mi listę tych ocen

Przykład

        public StudentController(IStudentService studentService, IGradeService gradeService)
        {
            _studentService = studentService;
            _gradeService = gradeService;
        }

        [HttpGet("{StudentId}/grades")]
        public async Task<IActionResult> GetGrades([FromRoute] GetStudentGradesRequest getStudentRequest)
        {
            var result = await _gradeService.GetStudentGrades(getStudentRequest);

            return result.Match<IActionResult>(error => error switch
            {
                DataNotFoundError _ => NotFound(error),
                _ => StatusCode(StatusCodes.Status500InternalServerError, error)
            }, _ => Ok(result.Data));
        }

    public class GetStudentGradesRequest
    {
        public int StudentId { get; set; }
    }

Czy powinienem do tego zastosować jakieś inne podejście w kwestii wrzucania serwisów do kontrolera? Jest jakiś bardziej pożądany/odpowiedni sposób?

2. Validacja id przy PUT

W tym samym kontrolerze wystawiam również endpoint PUT przyjmujący id i obiekt z nowymi danymi ucznia, w projekcie korzystam z FluentValidation i chciałbym zwrócić odpowiednią wiadomość, gdy użytkownik spróbuje uderzyć w endpoint z id < 1

        [HttpPut("{id:int:min(1)}")]
        public async Task<IActionResult> Update(int id, [FromBody]UpdateStudentRequest studentData)
        {
            var result = await _studentService.Update(id, studentData);

            return result.Match<IActionResult>(error => error switch
            {
                DataNotFoundError _ => NotFound(error),
                _ => StatusCode(StatusCodes.Status500InternalServerError, error)
            }, _ => Ok());
            
        }

Mam na to kilka pomysłów, ale żaden mnie nie satysfakcjonuje

• Nie informować użytkownika o tym, a korzystając (jak teraz) z constraintów uniemożliwić korzystanie z PUT dla takiego id
• Zrobić validację na początku metody Update (if id < 1 ...)
• Utworzyć walidację dla dto (Student) i po zmapowaniu sprawdzać czy spełnia warunki
• Pozwolić użytkownikowi uderzać w bazę i potraktować to tak samo jakby próbował dostać danę o uczniu z id, którego nie ma w bazie (notfound)

Jeśli przewidujesz że skończy się to dużą ilością serwisów wstrzykiwanych do kontrolerów, to najlepiej byłoby to rozbić. Z tym że ja bym w takim przypadku radził zastosowanie wzorca mediator, i delegowanie każdej operacji do odpowiedniego request handlera. Wtedy Twój kontroler jest "głupi" i nie interesuje go jak coś zrobić, tylko aby wysłać polecenie zrobienia czegoś. Jedyne za co wtedy kontroler jest odpowiedzialny to wysłanie odpowiedniego polecenia, oraz zwrócenie odpowiedniego statusu.

https://github.com/jbogard/MediatR

Czy jeden kontroler może (czy jest to wskazane) korzystać z kilku serwisów?

Generalnie nie jest to coś złego (imo), ale może od razu chcesz iść w kierunku MediatRa?

jakiś koncept przykład z neta

[HttpGet("{id}")]
public async Task<Order> Get(int id) 
{
	var user = await _mediator.Send(new GetUserDetailQuery(id));
	
	if user == null 
	{
		return NotFound();
	}
	
	return Ok(user);
}

public class GetUserDetailHandler : IRequestHandler<GetUserDetailQuery, UserDto> 
{
   	private readonly IUserRepository _userRepository;
    private readonly IUserMapper _userMapper;
    
    public GetUserDetailHandler(IUserRepository userRepository, IUserMapper userMapper)
    {
    	_userRepository = userRepository ?? throw new ArgumentNullException(nameof(userRepository));
    	_userMapper = userMapper ?? throw new ArgumentNullException(nameof(userMapper));
    }
    
    public async Task<UserDto> Handle(GetUserDetailQuery request, CancellationToken cancellationToken)
	{
		var user = await _userRepository.GetAsync(u => u.Id == request.Id)
		
		if (user == null)
		{
			return null; // wtf 
		}
		
		var userDto = _userMapper.MapUserDto(user);
		return userDto;
	}
}

W tym samym kontrolerze wystawiam również endpoint PUT przyjmujący id i obiekt z nowymi danymi ucznia, w projekcie korzystam z FluentValidation i chciałbym zwrócić odpowiednią wiadomość, gdy użytkownik spróbuje uderzyć w endpoint z id < 1

Nie informować użytkownika o tym, a korzystając (jak teraz) z constraintów uniemożliwić korzystanie z PUT dla takiego id
Zrobić validację na początku metody Update (if id < 1 ...)
Utworzyć walidację dla dto (Student) i po zmapowaniu sprawdzać czy spełnia warunki
Pozwolić użytkownikowi uderzać w bazę i potraktować to tak samo jakby próbował dostać danę o uczniu z id, którego nie ma w bazie (notfound)

Ja osobiście stosując wyżej wymienione podejście, to w metodzie Handle miałem również jakiś Validate, który np. walił do bazki, sprawdzał czy otrzymane daje są sensowne oraz spójne z systemem.

Proponowanie MediatR w sytuacjach takiego typu to nie jest żadne rozwiązanie bo tak jak już w komentarzu dopisał @WeiXiao - to jest wyłącznie przeniesienie potencjalnego problemu w inne miejsce.

Kontroler może korzystać z wielu serwisów, ale prawdę mówiąc im mniej tym lepiej.

Dla mnie te 2 konteksty powinny zostać rozdzielone. Wystawiłbym dwa osobne kontrolery do obsługi danych studenta oraz ocen i zaliczeń. A może nawet więcej bo czym innym są np. dane osobowe studenta a czym innym przypisanie do grup, obsługa ewentualnych płatności itd.

@var: nie zgadzam się z takim uproszczeniem, tak samo jak ze spotykanym przez @WeiXiao w internecie argumentem. Wprowadzenie wzorca mediator (MediatR to tylko biblioteka) i co za tym idzie odpowiednich handlerów to również wprowadzenie SRP, bo nagle od każdego procesu biznesowego mamy wyspecjalizowaną klasę, która również przyjmuje mniej zależności niż taki kontroler bo jedyne zależności to te związane z tym konkretnym procesem. Poza to część serwisów może nam wtedy zniknąć bo ich logikę można umieścić właśnie w handlarze. W dłuższej perspektywie znacznie to ulepsza architekturę w projekcie i czyni czytanie kodu dla konkretnego procesu znacznie czytelniejszym.

Tak więc powiedzenie że wprowadzenie mediatora to wyłącznie przeniesienie problemu w inne miejsce, czy też robienie tego samego co robi kontroler, to albo nie zrozumienie czym tak naprawdę jest wzorzec mediator i co wnosi, albo nadmierne upraszczanie.

Wystawiłbym dwa osobne kontrolery do obsługi danych studenta oraz ocen i zaliczeń. A może nawet więcej bo czym innym są np. dane osobowe studenta a czym innym przypisanie do grup, obsługa ewentualnych płatności itd.

Poza tym zwróć uwagę że tylko autor wątku wie o jakiej skali mowa. Pisanie do obsług płatności, obsługi zaliczeń itp. to już eksperyment myślowy bo nie wiadomo czy coś takiego będzie potrzebne. A nawet jeśli, to nadal można polemizować gdzie należało by umieścić endpoint do pobrania ocen studenta. A patrząc tylko na ścieżkę, np. students/{StudentId}/grades wydaje się mieć sens. Albo i nie. Tu akurat można polemizować jak już wspomniałem.

Rozwiązanie z mediatorem wydaje się ciekawe z tego względu, że kontroler ma mało zależności. Nie próbowałem takiego podejścia, więc nie wypowiem się "uczciwie", jednak wydaje mi się, że może powstać pewien problem. Chociaż wszystko zależy od punktu siedzenia. W tym momencie jedna klasa jest odpowiedzialna za jedną operację. Z jednej strony mamy SRP, ale z drugiej strony, czy nie jest to może pewna nadgorliwość? Wychodziłoby, że do zarządzania uczniami (dodawanie, modyfikowanie, usuwanie) potrzebowalibyśmy 3 klas zamiast jednej w stylu StudentService, która miałaby te 3 operacje zaimplementowane.

Poza tym nie musimy wstrzykiwać serwisu w konstruktorze. Można to zrobić wtedy, kiedy naprawdę jest potrzebny w taki sposób:

[HttpPost("endpoint")]
public async Task<IActionResult> PostDaSheet([FromBody]SheetDto dto, [FromServices]ISheetService service)
{

}

@Juhas: oczywiście, jak to ze wszystkim w programowaniu bywa- to zależy ;) Dałem okejke za propozycję ze wstrzykiwaniem serwisów bezpośrednio do action methods, mimo że sam nie jestem fanem wstrzykiwania zależności gdziekolwiek poza konstruktorami.

Odnosiłem się do użycia biblioteki MediatR.
Sam wzorzec mediatora również nie rozwiązuje problemu, który może być bardziej skomplikowany od tego jaki został zaprezentowany.
Wielokrotnie widziałem konstruktory-potwory które przyjmowały po 15 zależności, które w teorii nie powinny być ze sobą powiązane. Ukrycie ich i przerzucenie gdzie indziej nie stanowi wtedy żadnego rozwiązania problemu.

Ale tak - zbyt daleko wybiegam myślami.

Co do samej nomenklatury - handler czy service na pewnym poziomie koncepcyjnym może być dokładnie tym samym

Juhas napisał(a):

Wychodziłoby, że do zarządzania uczniami (dodawanie, modyfikowanie, usuwanie) potrzebowalibyśmy 3 klas zamiast jednej w stylu StudentService, która miałaby te 3 operacje zaimplementowane.

Jeżeli chodzi o zarządzanie bezpośrednio uczniem to tak, ale co gdy musimy chcemy dostać jego oceny lub inne dane 'dzieci'? To też umieściłbyś w StudentService?

enxnet napisał(a):

1. Czy jeden kontroler może (czy jest to wskazane) korzystać z kilku serwisów?

Może korzystać z wielu zależności, ale to szybko prowadzi do powstania god-objectu z mnóstwem zależności. Kontroler jest często wejściem dla wielu różnych przypadków użycia, każdy z nich może wymagać zupełnie innych zależności, a kontroler będzie musiał mieć wszystkie.
No chyba, że będziemy robić kontrolery z pojedynczymi akcjami. Tylko to leczenie objawu, a nie problemu.

A druga sprawa jest taka, że jeśli coś się nazywa Service to zazwyczaj jest to smrodek - taka klasa robi wszystko, i łamie wszystkie możliwe dobre praktyki. Klasy powinny być małe, a ich nazwy odpowiadać temu, co robią, a nie być tak generyczne i nic niemówiące jak "service".

Mam na to kilka pomysłów, ale żaden mnie nie satysfakcjonuje

• Nie informować użytkownika o tym, a korzystając (jak teraz) z constraintów uniemożliwić korzystanie z PUT dla takiego id
• Zrobić validację na początku metody Update (if id < 1 ...)
• Utworzyć walidację dla dto (Student) i po zmapowaniu sprawdzać czy spełnia warunki
• Pozwolić użytkownikowi uderzać w bazę i potraktować to tak samo jakby próbował dostać danę o uczniu z id, którego nie ma w bazie (notfound)

Myślę, że jak najbardziej powinieneś zwracać 404, tylko w takim przypadku nie musisz nawet uderzać do bazy.

Myślę, że jak najbardziej powinieneś zwracać 404, tylko w takim przypadku nie musisz nawet uderzać do bazy.

Lepiej sprawdzić to id w jakimś handlerze lub bezpośrednio przed wykonaniem zapytania?

Im wcześniej tym lepiej, więc najlepiej na poziomie akcji kontrolera. Jeśli "{id:int:min(1)}" wystarcza, to zostaw. Możesz też id zmienić na typ bez znaku. Możesz walnąć ifa na początku akcji.