Refactor DB generics #382
Refactor DB generics #382
Conversation
|
@alegacy, I believe this will simplify a lot the code. By moving the build of the query in the db repositories, we can easily construct more complex queries with many records. It looks cleaner. Please have a look |
Let's discuss next week. I must be missing something because this seems to have moved what was a relatively manageable set of generic functions back out to the repository and duplicates the logical in dozens of places. If any particular use cases needed a more complex query there was nothing stopping us from doing that on a case-by-case basis. Having the typical Find/Create/Delete cases handled generically seemed like a better approach short of using a proper ORM to do the heavy lifting. |
mlguerrero12
force-pushed
the
refactorgenerics
branch
2 times, most recently
from
9313a98 to
e40eb90
Compare
Provide helper Execute query functions to run more complex queries. Clean up existing basic CRUD operations Signed-off-by: Marcelo Guerrero <[email protected]>
mlguerrero12
force-pushed
the
refactorgenerics
branch
from
e40eb90 to
128880a
Compare
As we discussed internally, my main concerned was that it seemed we were maintaining a library here. There was no clear convention of what was a basic generic operation. I didn't want us in the future to continue adding arguments to fulfill all needs. For instance, if someone needed more flexibility with the upsert operations (which might be very complex itself) or add support for bulk operations with Insert or Update. I do understand your point about repeating code. Although, I believe it was ok in this case. So, as discussed, I left the basic crud operations and emphasized that Insert and Update are for a single record. I cleaned up a bit. I removed the generic functions for upsert. The queries for upsert are now built in the repo file. I also introduced 2 generic functions to execute queries which are used by the existing crud operations and can be used outside to run more complex queries. Please have a look again, @alegacy |
| records, err := utils.UpsertOnConflict[models.AlarmDictionary](ctx, ar.Db, []string{tags["AlarmDictionaryVersion"], tags["EntityType"], tags["Vendor"], tags["ResourceTypeID"]}, values) | ||
| tags := utils.GetDBTagsFromStructFields(dbModel, "AlarmDictionaryVersion", "EntityType", "Vendor", "ResourceTypeID") | ||
|
|
||
| columns := []string{tags["AlarmDictionaryVersion"], tags["EntityType"], tags["Vendor"], tags["ResourceTypeID"]} |
There was a problem hiding this comment.
It's really unfortunately that they sometimes accept any and other times they accept string because here we could have used tags.Columns()
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alegacy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Provide helper Execute query functions to run more complex queries. Clean up existing basic CRUD operations