Refactor Service method signatures

[carltonwhitehead]

Most current io module Service class methods follow the below pattern, not limited to create methods:

fun <E> create(entity: E) {
    persistConstraints.assess(entity)
    resource.create(mapper.toSnoozle(entity))
}

This causes users of the Service classes to take on too much responsibility for creation, mutation, etc of the entities. That logic belongs in the Service classes, not scattered around various UIs.

Proposal based on recent refactor of EventService:

fun create(
    id: UUID? = null,
    name: String,
    date: LocalDate,
    crispyFishEventControlFile: Path,
    crispyFishClassDefinitionFile: Path,
    motorsportRegEventId: String?,
    policy: Policy
): Event {
    val create = Event(
        id = id ?: UUID.randomUUID(),
        name = name,
        date = date,
        lifecycle = Event.Lifecycle.CREATE,
        crispyFish = Event.CrispyFishMetadata(
            eventControlFile = dbConfig.asRelativeToCrispyFishDatabase(crispyFishEventControlFile),
            classDefinitionFile = dbConfig.asRelativeToCrispyFishDatabase(crispyFishClassDefinitionFile),
            peopleMap = emptyMap() // out of scope for add command
        ),
        motorsportReg = motorsportRegEventId?.let { Event.MotorsportRegMetadata(
            id = it
        ) },
        policy = policy
    )
    persistConstraints.assess(create)
    resource.create(mapper.toSnoozle(create))
    return create
}

[carltonwhitehead]

Updated proposal for the method signature:

fun create(param: CreateParam): Result<Event>

Many of the entities contain as many or more properties as Event, and I expect these will only increase as the project builds out. It'll be cleaner in the long run to extract these to parameters. The previous pattern especially suffers from the problem of making it complicated to determine whether to instantiate nested objects, such as Event.CrispyFishMetadata, where the CreateParam could easily let users choose to exclude or defer setting up that metadata.

data class CreateParam(
    id: UUID? = null,
    name: String,
    date: LocalDate,
    crispyFish: Event.CrispyFishMetadata?,
    motorsportReg: Event.MotorsportRegMetadata?,
    policy: Policy
)

Notice this parameter excludes lifecycle. New Event entities will always be created with Event.Lifecycle.CREATE. This will allow club staff to avoid publishing information related to upcoming events before it would be appropriate. For example, a web-based results module would be configured to exclude any events below a minimum lifecycle status. I expect similar concerns may apply to other types. Why let the user pass an object with a value that definitely isn't allowed when we would just have to write constraint to reject it, when we could design out the possibility?

Changing the return type to Result will also help encourage UIs to account for error handling by explicitly placing the possibility of failure into the return type.

It would also be nice to adopt a standard CrudService interface throughout the project. Something like:

interface CrudService<Entity, CreateParam, ReadParam, UpdateParam, DeleteParam> {
    suspend fun create(param: CreateParam): Result<Entity>

    suspend fun read(param: ReadParam): Result<Entity>

    suspend fun update(param: UpdateParam): Result<Entity>

    suspend fun delete(param: DeleteParam): Result<Unit>
}

At time of writing, all of the services are just classes which loosely (inconsistently) follow pretty similar conventions, but there are definitely some outliers. Having some standardization would make them easier to work on, and simplify mocking.

There should also be a separate interface for services that support listing entities. This would be good to do in conjunction with #58. Example:

interface ListService<Entity> {
    suspend fun list(): Result<List<Entity>>
}

Breaking this out separately from the CrudService interface is meant to encourage implementations to think about the data type used for their listing entity. It likely doesn't make sense to use the full base Entity because it may have many relationships to other objects which don't make sense to load for a listing.