gilcrest / diygoapi Goto Github PK

Figure out how to integrate sqlc with project. Find out if sqlc can work within the datastore pattern I established or abandon it and just use sqlc.

Investigate why auth_token_access_token_expiry is null and determine default if not given by authentication provider. May be a bug, may be an enhancement - TBD

Add gRPC to the project and change structure to handle accordingly.

With enhanced routing patterns added in Go 1.22, move routing to standard library.

Unpredictability of these IDs is important - switch to use crypto/rand for randomness instead of xid

Tracking issue for:

• https://github.com/gilcrest/go-api-basic/security/code-scanning/2

Refactor broke tests. Fix them all....

• Under cmd/diy, cmd/migration, and cmd/worker are there.

Any suggestion on how to avoid the duplication across the main.go of cmd/diy/main.go and cmd/worker/main.go.

I see you have cmd/cmd.go but it wont work for different binaries? Should we have generic functions there?

Genesis request should not have Bearer token in the request body.

WithEnvVarNoPrefix from ff is deprecated, need to dig into library and update

Dig more into go-cloud health checkers and understand how they work. Right now, app is starting when db is down and there's no alerts, etc.

Instead of setting secrets (encryption key, db username/password) through environment variables in GCP. Figure out how to integrate with GCP's Secrets Manager and Cloud Run. I believe it's pretty straightforward, actually, and should get to this soon as it's important.

Need to allow for mapping an Oauth2 provider client ID to an App

Deletes were not throwing any error if nothing was deleted. Ensure that the sql actually affects 1 row.

Hey @rafaelbreno - sorry, by merging, I ended up closing the issue. If you don't mind, I'll just open a few new small issues that we can review some of the feedback from the tests you created? They look great by the way - thanks so much!

For the TestSetReleasedOk test, I believe the issue is that you're comparing the value from newReleased := time.Now(), which is a full time.Time with nanosecond precision. The SetReleased method expects a string in the RFC3339 format and parses as such:

func (m *Movie) SetReleased(r string) (*Movie, error) {
	t, err := time.Parse(time.RFC3339, r)
	if err != nil {
		return nil, errs.E(errs.Validation,
			errs.Code("invalid_date_format"),
			errs.Parameter("release_date"),
			errors.WithStack(err))
	}
	m.Released = t
	return m, nil
}

RFC3339 = "2006-01-02T15:04:05Z07:00"

RFC3339 does not have nanosecond precision, so when you send through the time.Time and format it, it's going to remove that precision.

gotMovie, _ = gotMovie.SetReleased(newReleased.Format(time.RFC3339))

and when you compare it again, you're comparing the original (newReleased) with nanosecond precision, with the return from the method without it.

	if gotMovie.Released != newReleased {
		t.Errorf("Want: %v\nGot: %v\n\n", newReleased, gotMovie.Released)
	}

Rather than use the time.Now() helper, I would send in a string as if you were receiving it from an API request in RFC3339 format. Parse that string in the test and then compare... Make sense?

Investigate libraries and implement one for Access Control, using either an Access Control List (ACL) or Role Based Access Control (RBAC) pattern

Tests are pretty useless with this data. Need to make test data better. Database setup is also necessary.

mi_losz on reddit pointed out "As for keeping the interfaces, if you import the interface from the datastore package, you're coupling the HTTP handler to this package. It can also lead to import cycles. Consider how you'd structure it if you'd like to introduce another method of saving models, like a NoSQL database"

I also was reading https://github.com/golang/go/wiki/CodeReviewComments#interfaces

Going to make changes to remove interfaces from datastore and other spots where it might make sense.

Tracking issue for:

• https://github.com/gilcrest/go-api-basic/security/code-scanning/1

I accidentalyl dropped creating the MovieService on server startup... add it back

https://blog.golang.org/wire

I spent some time with opencensus, opentrace and opentelemetry and decided to come back to this space later. I firmly believe in observability and plan to implement something, but feel this space has maturing to do in the open source world for Go.

RBAC is a very complicated subject. To keep things relatively simple, every user created through the /api/v1/users POST service will be granted the movieAdmin role in order to be able to create, read, update and delete movies through the API. The genesis user will be the only user granted the sysAdmin role which can call any service.

Need real handler tests

Currently fabricating a TraceID using xid - need to get the TraceID from whichever Telemetry library I go with

How would you go about sqlc on an sql query with a join that returns multiple rows .Maybe an inner join or left join.

After much thought - Peter Bourgon is correct. I do not need to use wire. I like wire a lot, actually and it helped me understand dependency injection better than I did before, but it's overkill for what I'm doing.

Determine how to connect to Google Cloud SQL from Google Cloud Run and document

Per lib/pq status, it is no longer resolving bugs or accepting new features, recommended to switch to pgx

Update to proper location

produced "OyXJ/BH2myP8g4ax" which has a forward slash, which brakes stuff...

mi_losz recommends Docker Compose for this - use either Docker Compose or Go itself or something else porter.sh?

Forgot to add the external id var to the url.

Right now, tests that hit the db are mocked. Need to add true tests that hit the db. It will be a significant effort to enable this due to the test data setup/cleanup that will need to be in place for these tests, but it's worth it.

Not needed - remove.

Instead of having create app id, create user id, create timestamp, etc. inside the Movie struct, create a separate struct as this concept should be used generally across all entities and tables.

If a database environment variable is not properly set, the error message is not showing the full error message.

I am already capturing a person's external Oauth2 bearer token in the HTTP Authorization header, but I also need to know what system/client/channel they are coming from.

Consider using X-APP-ID and an X-API-KEY (ref here) headers to denote what system/client a request is coming from.