Asset API #22

eirinivand · 2024-07-21T21:08:16Z

Will be squashed upon request.

- Fetch User Favourites - Remove User Favourite - All around fixes

- Add Remove fanourite endpoint - Add Audiences (bulk) endpoint - Add List Audiences endpoint - Map Audience, Insight or Chart to user's directly under Favourite - Add postman environment and collection - Add Mock data for Audiences - Update README

After successfully bringing up an independent container with swagger, the UI was not nice. - So we fall back to plain old postman environment and collections. - And update readme file - Fix Insight id and other minor changes - Fixes postman collection

wiz-gwi · 2024-07-21T21:08:29Z

Wiz Scan Summary


IaC Misconfigurations
Vulnerabilities
Sensitive Data
Total
Secrets

NikosMas

Hello @eirinivand 👋
Thank you very much for your time and effort in completing this assignment! I've added some comments, but I also have a few general questions about your implementation:

NoSQL Database Choice:
- Why did you choose to use a NoSQL database? Can you elaborate more on your thoughts behind this decision?
- Additionally, could you please describe, in simple terms, an approach to using an SQL database instead?
Models Package Usage:
- What are your thoughts on using the models package both in the endpoints' request bodies and in the database structs?
- Do you see any drawbacks to this approach?
Testing Priorities:
- If you had to add some tests to your implementation, which parts would be your priorities?

NikosMas · 2024-07-26T06:17:17Z

database/user_service.go

+	GetByID(ctx context.Context, id string) (models.User, error)
+	Create(ctx context.Context, m *models.User) error
+	CreateAll(ctx context.Context, m []*models.User) error
+	Update(ctx context.Context, id string, m models.User) (int64, error)


In the Create and CreateAll methods, your User inputs are pointers, but not in the Update one. Is there any specific reason for that??

Well, the thing is, I forgot to use and update these, so there is no reason for it; it should have been a pointer. 😅

NikosMas · 2024-07-26T06:22:56Z

handlers/asset_handler.go

+	return &AssetHandler{service: service}
+}
+
+func (h *AssetHandler) GetAll(ctx *gin.Context) {


I noticed in your implementation that the context is propagated end-to-end until the database. What are some advantages of doing this?

Timeounts: say something takes too long it can be terminated at any level, or if the request is cancelled.

Middleware usage, say to set/check/unset cookies

That being said also security, since in an extension cookies/tokens can be checked at any level.

Keeping the data relative to the request for improved consistency.

NikosMas · 2024-07-26T06:27:05Z

models/asset.go

+type AssetInterface interface {
+	GetId() primitive.ObjectID
+	Description() string
+	GetAssetType() AssetInterface


What's the point of this method?
In the implementations (audience, chart, insight), the actual object is returned

This was forgotten in there (and in the implementations). I was experimenting with different approaches to fetching the object. This one was not useful and was thus abandoned.

NikosMas · 2024-07-26T06:50:30Z

utils/db.go

+	return client
+}
+
+func CloseClientDB() {


I noticed that you don't call the CloseClientDB method anywhere in your implementation. Is there any specific reason for that?

True, forgotten, should have been during application shutdown.

eirinivand · 2024-07-26T10:27:19Z

Hello @NikosMas 👋

Thank you for your comments! Here are the answers to the more general questions:

NoSQL Database Choice:
- Why did you choose to use a NoSQL database? Can you elaborate more on your thoughts behind this decision?
  - Nulls: Since my models have different attributes that can be nil depending on features' usage of each asset. and to avoid the many nulls in an SQL DB. For example, I initially planned to make Charts have a Z-axis as well, which I would simply have nil in case of 2D charts. In which case I would also have Points struct with a Z field.
  - Ease: Also given the simplicity of fetching such objects, I preferred it. Cause joining different tables in SQL would result to different Assets Type objects, that I would have to do extra effort to fit under the favourite.asset field. This approach simplifies it significantly.
- Additionally, could you please describe, in simple terms, an approach to using an SQL database instead?
  - Models: Well the models would be more strict in terms of fields, and I would spread them across different tables. For example Audience's attributes or Chart's Points would have been different tables using join tables to connect them.
  - Queries: Utilize the join tables, which is usually a pain to do with ORMs (I do have a soft spot for plain old SQL queries).
  - Mapping: Well here is the issue of your point (2). This would require mappers and different interfaces to achieve the same result as my current solution.
Models Package Usage:
- What are your thoughts on using the models package both in the endpoints' request bodies and in the database structs?
  - Well, ok, not the best approach but given my choices, it works, I do hide some fields on the json or bson part in order to address different problems, e.g., not returning the password hash during the fetching of the user for the GetByUsername 😅 . This should not be the case in a larger-scale app.
- Do you see any drawbacks to this approach?
  - As explained above, having different models for the endpoints vs. the database has various benefits, like adding elements to the endpoint that need not be stored or require different handling, e.g., validation. If, for example, I wanted to persist an 'F' for Gender instead of the entire word, what kind of validation of field would I have? Then I would make use of different structs.
Testing Priorities:
- If you had to add some tests to your implementation, which parts would be your priorities?
  - Unit Tests for exposed functions
  - Endpoints, Input (validation), and Responses (status codes for different occasions).

eirinivand added 8 commits July 17, 2024 23:38

Init commit

89c8a0a

Fix favourites add

d18692d

Fixed JWT

034da49

Various additions:

a69c7b6

- Fetch User Favourites - Remove User Favourite - All around fixes

Add mock Chart and insight data, fix admin, update postman collection

cd38970

Updated field names

406a40b

Add documentation

f0518fd

After successfully bringing up an independent container with swagger, the UI was not nice. - So we fall back to plain old postman environment and collections. - And update readme file - Fix Insight id and other minor changes - Fixes postman collection

eirinivand marked this pull request as ready for review July 21, 2024 21:25

NikosMas reviewed Jul 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset API #22

Asset API #22

eirinivand commented Jul 21, 2024

wiz-gwi bot commented Jul 21, 2024

NikosMas left a comment

NikosMas Jul 26, 2024

eirinivand Jul 26, 2024

NikosMas Jul 26, 2024

eirinivand Jul 26, 2024

NikosMas Jul 26, 2024

eirinivand Jul 26, 2024

NikosMas Jul 26, 2024

eirinivand Jul 26, 2024

eirinivand commented Jul 26, 2024

Asset API #22

Are you sure you want to change the base?

Asset API #22

Conversation

eirinivand commented Jul 21, 2024

wiz-gwi bot commented Jul 21, 2024

Wiz Scan Summary

NikosMas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirinivand commented Jul 26, 2024