USACE / cwms-data-api

Corps Water Management System RESTful Data Service
MIT License
11 stars 14 forks source link

CwmsDtoBase vs CwmsDto design #768

Open MikeNeilson opened 1 month ago

MikeNeilson commented 1 month ago
          Adding the required property is going to break other DTO's that are outside the scope of just the Outlet DTO's.  This has to do with the validator piece that was added, and the annotation reflection it does to check for required = true.  I think this needs to be a broader conversation on the design for this object, including name changes since it's not consistent.

I can create a ticket for that, but adding the annotation back in will be problematic right now.

_Originally posted by @RyanM-RMA in https://github.com/USACE/cwms-data-api/pull/731#discussion_r1670885826_

MikeNeilson commented 1 month ago

Perhaps CwmsDtoBase should have a getOffice function that CwmsDto can have a default implementation of. At a certain point the DAO need a reference office to set the session permissions, when I first designed that I hadn't considered the nest objects, changing that to a function that downstream implementations provide is probably the best thing.

adamkorynta commented 1 month ago

I only see two references to the Dao.setOfficeId(Context, CwmsDTO) method. Everything else uses the Dao.setOfficeId(Context, String) overload. Is there somewhere else that I'm not seeing that relies on the office for session permissions?

MikeNeilson commented 1 month ago

Dang it man, stop pointing out my obvious design failures! :).

Anyways, every call to a non-readonly operation requires the session office being set. We determined the best way was at the point of the Dao's usage of the connection (as it would actually be an active connection that will be directly used.)

Since every DAO was taking a CwmsDto Ryan or I likely made the CwmsDTO over to simplify the operations, for the String overload that was probably for testing.

I would argue that:

  1. We should have a standard DTO method to retrieve the appropraite office for the object (I think some of the aggregate objects get.. odd, plus in the future office might actually be used for responsibility vs "which districts data")
  2. The DAOs when setting the offset should use the CwmsDto overload, unless it really doesn't work.
  3. We should put a note on the string overload if that really is just for testing.

So I think I propose changing CwmsDto to


public interface CwmsDto extends CwmsDtoBase {
    @JsonIgnore
    String getSessionOffice();
    @JsonIgnore
    CwmsId getCwmsId(); // maybe, implementing DTOs can tell the parsers to split as appropriate but with these two it seems we can properly identify everything.
}
MikeNeilson commented 1 month ago

NOTE: approved, but definitely needs some discussion first.

adamkorynta commented 1 month ago

Something else to note: every post would have a DTO with a session office, but delete and rename operations only use query parameters.