Dev #16
Dev #16
Conversation
Wiz Scan Summary
|
| GivenName string | ||
| Surname string | ||
| Password string | ||
| Favorites []Favorite |
There was a problem hiding this comment.
If we are talking about the Favorites []Favorite attribute, the reason why I’ve added this field here is to express in the “gorm-way” a has-many relationship between User and Favorite entities.
I wanted to utilize as much as I could the provided functionality of the ORM that I’ve decided to use (gorm). The relationship that I wanted to define was that a user has many favorites. Based on the documentation here this was the proper way to do this, based on my understanding. I was planning to use something along these lines err := db.Model(&User{}).Preload(“Favorites”).First(&user, uid) at some point but I ended up doing something else. However, the above User struct definition enabled gorm to create the proper foreignKey automatically on my behalf in the favorites table.
| db gorm.DB | ||
| } | ||
|
|
||
| func NewUserService(db gorm.DB) (UserService, error) { |
There was a problem hiding this comment.
Busted :)
Definitely something is missing here! My intentions were to check that I have an actual instance of the connection to the db and if not to throw an appropriate error. Something like
if db == nil { return nil, fmt.Errorf("database connection is nil") }
As I can see now, I have forgotten to do so. The same error exists in every service instantiation. I am sorry this is my bad
| // TODO: Here first check if we have items in cache and return them, if not proceed with hitting the DB | ||
| // (take into consideration the pagination params) | ||
|
|
||
| fs.db.Model(&models.Favorite{}).Where("user_id = ?", uId).Count(&count) | ||
|
|
||
| if err := fs.db.Joins("Asset").Where("user_id = ?", uId).Order("asset_id asc").Order("updated_at desc").Find(&favorites).Error; err != nil { | ||
| return pr, err | ||
| } |
There was a problem hiding this comment.
Could you describe a different way of how a service could handle all these details?
There was a problem hiding this comment.
A better approach could be to group all the logic which interacts with the data layer inside its corresponding entity’s repository, e.g. UserRepository,AssetRepository, etc. by doing so we can abstract the details of interacting with the data layer enabling better modularity.
The repository pattern could also hide technical choices like which specific ORM is currently in use by just exposing functions like FindAllAssets(), FindAllFavorites(), etc. This could enable better flexibility in the sense that at any time ORM can change with minimum implications.
| return &assetService{db: db}, nil | ||
| } | ||
|
|
||
| func (as *assetService) GetAssets(p int, pS int, t string) (serializers.AssetsResponse, error) { |
There was a problem hiding this comment.
If you had to test the following implementation how would you do that?
There was a problem hiding this comment.
To be 100% honest the way it is structured now makes it a bit difficult to test. A second iteration would have revealed that :) A proper dependency injection would have been a better solution here. So a dependency for the service would have been its corresponding repository. This approach would allow to mock calls to the corresponding repository/ies and focus on better testing the behavior of service
| "gorm.io/gorm" | ||
| ) | ||
|
|
||
| func AggregateAssetType(a models.Asset, db gorm.DB) (*models.AssetResponse, error) { |
There was a problem hiding this comment.
In your opinion why would such a file be part of a helpers package?
There was a problem hiding this comment.
My initial thought was that I wanted to extract that functionality somewhere as I am using it in a variety of cases, so I wanted something reusable. As I haven’t followed the repository pattern anywhere I thought that placing this under the helpers folder would be fine given the rationale that I already had the pagination helper there which was interacting with the database too. I see now why it is not the most obvious thing to do. As this function is heavily relying on the ORM a better place for it could be inside an AssetRepository where all the interactions with the ORM should be moved. This applies to all my entities
| Title string | ||
| HorizontalAxisLabel string | ||
| VerticalAxisLabel string | ||
| Data pq.Float64Array `gorm:"type:float8[]"` |
There was a problem hiding this comment.
Can you think of any concerns of having the model so strongly coupled with the database?
There was a problem hiding this comment.
Coupling a model directly with the database can limit testability, reduce flexibility, complicate refactoring, and violate separation of concerns *. This makes the code harder to maintain and evolve. Things like the repository pattern and dependency injection as discussed above can make a big difference moving to the right direction.
*however it is acceptable to have the management of transactions in the service layer if you want to perform multiple dependent operations and have the ability to roll them back upon failure
| RelatedType string | ||
| } | ||
|
|
||
| type AssetResponse struct { |
There was a problem hiding this comment.
Yes I wanted to represent in a specific way the data that returned as a response to the client. I see now that maybe this should be part of the serializers and don’t belong here or even it is not the most appropriate way to deal with this type of data transformations.
|
Thank you @nmakro for your feedback! I truly appreciate the time you spent writing these comments. Each of your questions is valid and to the point. A second iteration would have been beneficial when it comes to my deliverable, as I was considering many of the points you raised. Unfortunately, I didn't have the time for it due to the deadline, as I aimed to display my ability to produce a full-stack PoC as soon as I could. |
No description provided.