ugent-library / old-people-service

People service
Apache License 2.0
0 stars 0 forks source link

[OpenAPI] Turn models into first class citzens, decouple from protobuf definition #29

Closed netsensei closed 1 year ago

netsensei commented 1 year ago

Story

Currently, the models in models/ are embedding the structs generated from the protobuf definition e.g.

import  v1 "github.com/ugent-library/person-service/api/v1"

type Person struct {
    *v1.Person
}

This creates a tight coupling between the proto generated Person struct used in gRPC context, and the Person model as used by the repository.

So, the goal of this story is to de-couple the model used in the repository from these protobuf generated structs.

Success criteria

Implementation suggestion

Turn the models into a first class citizen by giving them the same properties as defined in the openapi.yaml e.g.

type Person struct {
    Id                 string
    GismoId            string
    Active             bool
    DateCreated        string
    DateUpdated        string
    FullName           string
    FirstName          string
    LastName           string
    Email              string
    Orcid              string
    OrcidToken         string
    PreferredFirstName string
    PreferredLastName  string
    BirthDate          string
    Title              string
    OtherId            []string
    Organization       []Organization
    JobCategory        []string
    Role               []string
    Settings           map[string]string
    ObjectClass        []string
    ExpirationDate     string
}

In the implementation of the methods Service struct in api/v1/service.go you need to convert from the models.Person to api.Person. You could leverage type conversions for this. If the underlying type of both types is identical (having identical properties) it should be possible to do this: (Assuming you have an ogen / OpenAPI generated type ResponsePerson struct)

p := repo.GetPerson(id)
rp := ResponsePerson(p)
return rp

By using type conversions provided by the compilers, you don't have to explicitly map properties like this:

rp := ResponsePerson {
  GismoId: p.GismoID,
  Active: p.Active,
  ...
}

You will also have to check for all references to v1.Organization and v1.Person across the codebase.

Automatic testing scenario

n/a

Additional information

nicolasfranck commented 1 year ago

@netsensei I cannot do this separately from #24. e.g. as soon as I remove v1.Person from the model models.Person all grpc code stops working because that expects v1.Person

netsensei commented 1 year ago

@nicolasfranck Okay. Let's close this issue and move your code under #24. Removing the gRPC code means you'll need to create a model.Person anyways.