[#624] Upgrade dependencies

Description

This PR upgrades go.mod packages to their latest versions (except github.com/getkin/kin-openapi, which may have an issue validating example requests in the latest version).

Resolves #624 Resolves #593

Review Time Estimate

Please give your idea of how soon this pull request needs to be reviewed by selecting one of the options below. This can be based on the criticality of the issue at hand and/or other relevant factors.

[ ] Immediately
[ ] Within a week
[x] When possible

Type of changes

Please select a relevant option:

[ ] Bug fix (non-breaking change which fixes an issue).
[ ] New feature (non-breaking change which adds functionality).
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected).
[x] Other (any another change that does not fall in one of the above categories.)

Checklist:

Please select all applicable options:

[x] I have signed the Rokwire Contributor License Agreement (CLA). (Any contributor who is not an employee of the University of Illinois whose official duties include contributing to the Rokwire software, or who is not paid by the Rokwire project, needs to sign the CLA before their contribution can be accepted.)
[x] I have updated the CHANGELOG.
[x] I have read the Contributor Guidelines.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] My change requires updating the documentation.
[x] I have made necessary changes to the documentation.
[ ] I have added tests related to my changes.
[x] My changes generate no new warnings.
[x] New and existing unit tests pass locally with my changes.
[x] Any dependent changes have been merged and published in downstream modules.

Thanks @petyos,

Thanks for catching this. I did not see that the kin-openapi package was updated to v0.113.0. This version changes the definition of readOnly from the definition that we have been operating under to this point. v0.110.0 seems to me to be the most recent version that does not change this definition, so downgrading the version in go.mod should allow you to run successfully. I am not sure whether we want to upgrade to v0.113.0, but if so, we will need to update our schemas to fit the new readOnly definition.
I think I understand what you are saying about the ..Fields.yaml schemas, and I agree with your comment about splitting our schemas into 3 categories. The reason why I removed the existing ..Fields.yaml schemas is because it seemed to me to be straightforward to merge them into the standard model schemas given the request and response schemas expected by the current version. You are correct about the modified LoginSession schema though, and I will go back and make sure it matches the response sent by the current version of the Core BB. I will also check to make sure the other schemas I changed match the behavior of the current version of the Core BB.

I think it is fine if we need to add new model schemas for new APIs that are slightly different from existing model schemas. This would be similar to PartialAccount.yaml, which is an account model, but slightly different from Account.yaml.

Please let me know what you think about this. Thanks!

Hi @roberlander2 ,

1. I think for now downgrading to v0.110.0 should be good.

I think it is fine if we need to add new model schemas for new APIs that are slightly different from existing model schemas. This would be similar to PartialAccount.yaml, which is an account model, but slightly different from Account.yaml.

Basically we already have all needed types of schemas - Model schemas,APIs requests schemas and APIs responses schemas.

Model schemas These schemas are a mirror of the structs which are placed in the model package. The reader can understand the model without reading Golang code but directly in the documentation.These schemas are not related with APIs. The only relation is that we reuse the ...Fields.yaml files for responses but this is just for files reusing as we mentioned. If the reader wants to call the Core BB APIs he should look at the schemas in the following two sections(requests and responses) below but not the model schemas whose purpose is to explain the data model only.
APIs Requests and Responses schemas For very simple data models the model schemas and the requests and responses schemas will be the same but for more complex models which is our case in Core BB they differ. Various requests and responses could have a substructure from the main model as we have in our APIs.

Regarding PartialAccount.yaml

If we want to be correct the place for PartialAccount.yaml is not in the model schemas but it should be in the responses schemas section. The reason is that in the model package in Golang we do not have a PartialAccount struct. This entity is needed for some of our APIs as a response. I do not say that we need to move this now in the response but I just want to be sure you understand what I mean. Imagine that we need another API which requires us to expose another substructure of our Account struct - PartialAccount2 etc.

Please let me know if you ahve any questions or comments. Thanks!

Hi @roberlander2 and @petyos, thanks for the discussion and feedback!

1. I think for now downgrading to v0.110.0 should be good.

I think we need to find a way to use the latest version of the package with the new definition of readOnly or else we will not be able to stay up to date with the latest changes going forward. My opinion is that it is better to take care of this now while making related changes than to put it off and have to deal with it again later.

Basically we already have all needed types of schemas - Model schemas,APIs requests schemas and APIs responses schemas.

Model schemas These schemas are a mirror of the structs which are placed in the model package. The reader can understand the model without reading Golang code but directly in the documentation.These schemas are not related with APIs. The only relation is that we reuse the ...Fields.yaml files for responses but this is just for files reusing as we mentioned. If the reader wants to call the Core BB APIs he should look at the schemas in the following two sections(requests and responses) below but not the model schemas whose purpose is to explain the data model only.

APIs Requests and Responses schemas For very simple data models the model schemas and the requests and responses schemas will be the same but for more complex models which is our case in Core BB they differ. Various requests and responses could have a substructure from the main model as we have in our APIs.

Regarding PartialAccount.yaml

If we want to be correct the place for PartialAccount.yaml is not in the model schemas but it should be in the responses schemas section. The reason is that in the model package in Golang we do not have a PartialAccount struct. This entity is needed for some of our APIs as a response. I do not say that we need to move this now in the response but I just want to be sure you understand what I mean. Imagine that we need another API which requires us to expose another substructure of our Account struct - PartialAccount2 etc.

Thanks @petyos for the detailed explanation of your view of the organization for the schemas! This is very clear and makes it easy for us to understand where the disconnect is. We have been thinking about the schemas differently from how you described them. Here is our understanding and some explanation of the reasoning for it:

Schemas

Model schemas: These schemas reflect the standard representation of a model in our system from the perspective of the API caller. They may or may not match the Go model structs in our model package exactly depending on whether all fields we store internally are exposed through the APIs. These should be reusable components that are used directly throughout the docs to represent this data type in the requests/responses to APIs.
APIs Requests and Responses schemas: If the schema of the data to be sent in an API request/response differs from our existing model schema we can create a schema specific to this API to reflect this change. This should not be as common as using the model schemas directly in the request/response, and these should never be an exact duplicate of a model schema.

Regarding PartialAccount.yaml

While I believe the naming of this type is not ideal, this was meant to reflect a standard schema that can be sent through any API that should only expose the minimum amount of personal information about an account necessary to identify who the account belongs to. This is intended to be used in any and all APIs that expose accounts to admins or anyone other than the user themself who should not access their full profile. We should not be making a different version of this model for each similar API, but rather reusing this to ensure we are enforcing the same limitations on the data available. A better name for this model could be AdminRestrictedPIIAccount or something similar. If we wanted to expose a different subset of account data on a new API we should create a similar model with a meaningful and distinct name so we know when to use one vs. the other.

Explanation

We realize that this understanding differs from yours in several key ways, but here is an explanation of why we still prefer this definition.

Maintainability: If we have a separate identical schema for each request/response that means that if we change a model we will need to go back and update every individual schema which will take more time and make it very difficult to ensure that all of the necessary schemas are updated. As the number of APIs grow it will be hard to remember every single individual API that uses a given model.
Development speed: Making separate schemas for each request/response will slow down development and require us to do additional work to create a new schema whenever we build a new API, even if an identical schema already exists.
Clarity: While I do understand your point about using the models section to understand the internal Go structure of the model, I feel that this is not entirely necessary since these are the API docs and if a field is not exposed to the caller through an API then there is no reason we need them to be aware of these internal implementation details. In my view it is best to only present the information about the interface in these docs to keep them clear and focused.
Consistency: If we use a different schema for every request/response even if it is the same model being sent then my feeling is that the API docs will be more difficult to interpret. For example, the caller loses the ability to easily identify that it is an Account being sent to them and instead they see something with a non-meaningful name that just reflects the API they are calling. They will need to look through all of the individual fields to understand that the model being used between two APIs is actually the same model which will make it more difficult for them to know what models are necessary to build on their end.
Readability: If we have many duplicate request/response schemas, we will end up cluttering up the models section of our docs which will mean that readers have to look through a longer list to find the models they are looking for.

I realize that this is a larger conversation and that there is no definitive correct strategy to follow here, but hopefully this is helpful for you to understand the intention behind the changes we made. Please let us know what you think of the organization we proposed here and the explanation we provided. Please also let us know If you have any questions or disagree with any of these points, or if we misunderstood anything about your explanation.

Thanks again for your input!

Hi @shurwit , Thanks for your explanation.

The big difference comes that we give different meanings of the three sections of schemas.

Schemas • Model schemas: These schemas reflect the standard representation of a model in our system from the perspective of the API caller. They may or may not match the Go model structs in our model package exactly depending on whether all fields we store internally are exposed through the APIs. These should be reusable components that are used directly throughout the docs to represent this data type in the requests/responses to APIs.

You treat them as part of the APIs context but I treat them as explaining the whole model for the reader - API callers, architects etc For API callers perspective I treat only the bellow schemas:

APIs Requests and Responses schemas: If the schema of the data to be sent in an API request/response differs from our existing model schema we can create a schema specific to this API to reflect this change. This should not be as common as using the model schemas directly in the request/response, and these should never be an exact duplicate of a model schema. Regarding PartialAccount.yaml While I believe the naming of this type is not ideal, this was meant to reflect a standard schema that can be sent through any API that should only expose the minimum amount of personal information about an account necessary to identify who the account belongs to. This is intended to be used in any and all APIs that expose accounts to admins or anyone other than the user themself who should not access their full profile. We should not be making a different version of this model for each similar API, but rather reusing this to ensure we are enforcing the same limitations on the data available. A better name for this model could be AdminRestrictedPIIAccount or something similar. If we wanted to expose a different subset of account data on a new API we should create a similar model with a meaningful and distinct name so we know when to use one vs. the other.

I think you will agree that our entity in the system is the Account entity - this is what our system supports. Yes, we can construct different entities(substructures) from the Account entity for the needs of the APIs but they are only on API level. Once passed to the system it is an Account entity. I think you are trying to identify such substructures which will be reused in many APIs and give them a name(AdminRestrictedPIIAccount is an example of such a substructure). If a substructure is used in many APIs then you put it in Model Schemas but if it is used once then you put it in APIs Requests and Responses schemas.

Explanation We realize that this understanding differs from yours in several key ways, but here is an explanation of why we still prefer this definition. 1 Maintainability: If we have a separate identical schema for each request/response that means that if we change a model we will need to go back and update every individual schema which will take more time and make it very difficult to ensure that all of the necessary schemas are updated. As the number of APIs grow it will be hard to remember every single individual API that uses a given model.

Maybe I did not succeed to explain it well but we will not have separate identical schemas for each request/response. AdminRestrictedPIIAccount can be defined in the APIs schemas in the same way and can be reused by as many APIs as needed. Something more - if an API needs to return for example the full structure of the Account entity(but not a substructure as AdminRestrictedPIIAccount) then it can use AccountFields.yaml. So, for API caller perspective we will have the following options which to be used in the APIs requests and responses for the Account entity AccountFields.yaml - all simple fields for the account (reused by many APIs) (yes, maybe the name is not 100% accurate) AdminRestrictedPIIAccount.yaml - substructure of the account fields (reused by many APIs) etc

Development speed: Making separate schemas for each request/response will slow down development and require us to do additional work to create a new schema whenever we build a new API, even if an identical schema already exists.

Read "Maintainability" comment

_Clarity: While I do understand your point about using the models section to understand the internal Go structure of the model, I feel that this is not entirely necessary since these are the API docs and if a field is not exposed to the caller through an API then there is no reason we need them to be aware of these internal implementation details. In my view it is best to only present the information about the interface in these docs to keep them clear and focused.

It is not about a field exposed in the docs. In this case these fields are a composition of other entities explaining the model in the system. I agree that this is extra information but this is not at the expense of the API docs as all needed information for API caller is already there. Also when the API caller looks at a specific API doc he looks at its section where he can see all - what to send, what to expect as response etc. The schemas below are just a list of schemas instead of an entry point for the API caller looking for a specific API doc. Also supporting these model explaining schemas is not a double work as we reuse it with ...Fields.yaml files.

Consistency: If we use a different schema for every request/response even if it is the same model being sent then my feeling is that the API docs will be more difficult to interpret. For example, the caller loses the ability to easily identify that it is an Account being sent to them and instead they see something with a non-meaningful name that just reflects the API they are calling. They will need to look through all of the individual fields to understand that the model being used between two APIs is actually the same model which will make it more difficult for them to know what models are necessary to build on their end.

Read "Maintainability" comment

Readability: If we have many duplicate request/response schemas, we will end up cluttering up the models section of our docs which will mean that readers have to look through a longer list to find the models they are looking for.

Read "Maintainability" comment

I agree it is a long conversation and there is no definitive correct strategy. As we are already in an advanced stage of development I would suggest to mix our understandings with minimal changes. I would suggest: Let's have 4 types schemas - 3 related with APIs and 1 for the Data model of the system: APIs data model This is general what meaning you currently give to the current model schemas. So here we would have: AccountFileds.yaml - all simple fields of the Account entity(without composition with other entities) AdminRestrictedPIIAccount.yaml - some substructure of the Account entity etc APIs Requests and Responses schemas: Specific schemas for specific requests and responses Data model schemas: It represents the data model of the system and is a mirror of it. It could Have AccountFields.yaml + all other entities compositions

Account
{
fields AccountFields.yaml

+ all composition fields
app_org ApplicationOrganization
...
}

///////////// What do you think about this? Thanks.

Hi @petyos, thanks for the suggestion and clarification!

As a side note we fixed the issue preventing us from upgrading to the latest version of kin-openapi.

I agree it is a long conversation and there is no definitive correct strategy. As we are already in an advanced stage of development I would suggest to mix our understandings with minimal changes. I would suggest: Let's have 4 types schemas - 3 related with APIs and 1 for the Data model of the system: APIs data model This is general what meaning you currently give to the current model schemas. So here we would have: AccountFileds.yaml - all simple fields of the Account entity(without composition with other entities) AdminRestrictedPIIAccount.yaml - some substructure of the Account entity etc APIs Requests and Responses schemas: Specific schemas for specific requests and responses Data model schemas: It represents the data model of the system and is a mirror of it. It could Have AccountFields.yaml + all other entities compositions
Account
{
fields AccountFields.yaml

+ all composition fields
app_org ApplicationOrganization
...
}
///////////// What do you think about this? Thanks.

I think I understand what you are saying better now. I still don't fully see the need for the "Data model schemas" but I think the strategy you proposed to manage them is a good one. If we are going to continue supporting these I would like to propose a small modification to the structure you outlined to help keep things organized and make the purpose of each schema clear. Here is the file structure I would suggest for the schemas/ directory:

fields/
  user/
    Account.yaml (name: AccountFields)
web/
  models/
    user/
      Account.yaml (name: Account)
  apis/ (matches the current docs/schemas/apis/)
internal/
  user/
      Account.yaml (name: InternalAccount)

fields: Contains the root level primitive fields (no composition) for the model to be used in both web/models/ and internal/ schemas. It is also acceptable to reference these directly from the API definitions if they match the exact requirements.
web: Contains all schemas that are used in APIs. Note that these schemas cannot contain recursive/circular references.
- models: Contains all standard models for use in APIs and are composed of fields and any additional IDs or other properties using the allOf keyword to merge and flatten the elements.
- apis: Same as the existing request/response schemas. Contains schemas that are specific to an individual API request/response. Should only be used if no standard web/model fits. shared requests/responses should be used if the type is not a standard model, but is used by multiple APIs (eg. across services and admin endpoints).
internal: Schemas that reflect the internal structs for standard models composed of fields and other internal models using the allOf keyword to merge and flatten elements. These are for informational purposes only and should not be used directly in API definitions. Because of this, these schemas may contain recursive/circular references, but should be ignored when generating Go types.

Note: No schemas should ever be exact duplicates.

Hi @petyos, I started attempting to implement the structure I described above and I am woried that it will still introduce a lot of extra work just to maintain the internal models since even with the shared fields we will need to essentially define every model twice and remember to update both when any changes occur.

Could you please explain why you believe it is important to maintain the internal models even though they are not being used in the APIs and this information is not really needed by the caller? While it may not detract from the quality of the docs, it does make the process of defining and maintaining these docs significantly more complicated and time consuming. I am still having a hard time understanding what benefit these provide that make them worth the extra effort.

Hi @petyos, I started attempting to implement the structure I described above and I am woried that it will still introduce a lot of extra work just to maintain the internal models since even with the shared fields we will need to essentially define every model twice and remember to update both when any changes occur.

Could you please explain why you believe it is important to maintain the internal models even though they are not being used in the APIs and this information is not really needed by the caller? While it may not detract from the quality of the docs, it does make the process of defining and maintaining these docs significantly more complicated and time consuming. I am still having a hard time understanding what benefit these provide that make them worth the extra effort.

Hi @shurwit, my point of view was that we already have data model schemas which cover the data model schemas in the way I treat it at acceptable level of implementation. But indeed, introducing internal at this stage will trigger a lot of changes. Feel free to ignore the Data model schemas/internal as you do not see a reason to support it. It was an extra information.

Hi @shurwit, my point of view was that we already have data model schemas which cover the data model schemas in the way I treat it at acceptable level of implementation. But indeed, introducing internal at this stage will trigger a lot of changes. Feel free to ignore the Data model schemas/internal as you do not see a reason to support it. It was an extra information.

Hi @petyos, thanks for the explanation and for approving. I see what you mean about these data model schemas already existing. We were trying to simplify things and reduce the amount of documentation overhead going forward, and I do feel that maintaining them will complicate things. Hopefully you agree that this will simplify things. Thanks again!

rokwire / core-building-block