From 753a8326741d34f92875a421c168699dfa1e0fd2 Mon Sep 17 00:00:00 2001 From: Brian Holt Date: Mon, 17 Nov 2025 21:26:33 -0600 Subject: [PATCH 1/3] improve trace instrumentation --- .../postgres/init/aws/SecretsManagerAlg.scala | 44 ++++--------------- .../repositories/CreateSkunkSession.scala | 35 ++++++++------- 2 files changed, 28 insertions(+), 51 deletions(-) diff --git a/src/main/scala/com/dwolla/postgres/init/aws/SecretsManagerAlg.scala b/src/main/scala/com/dwolla/postgres/init/aws/SecretsManagerAlg.scala index 9c39e1d..721d432 100644 --- a/src/main/scala/com/dwolla/postgres/init/aws/SecretsManagerAlg.scala +++ b/src/main/scala/com/dwolla/postgres/init/aws/SecretsManagerAlg.scala @@ -5,6 +5,7 @@ import cats.* import cats.effect.{Trace as _, *} import cats.syntax.all.* import cats.tagless.aop.* +import cats.tagless.Derive import com.amazonaws.secretsmanager.* import com.dwolla.tagless.WeaveKnot import com.dwolla.tracing.syntax.* @@ -15,46 +16,19 @@ import org.typelevel.log4cats.Logger trait SecretsManagerAlg[F[_]] { def getSecret(secretId: SecretIdType): F[Secret] - def getSecretAs[A : Decoder](secretId: SecretIdType): F[A] + def getSecretAs[A : {Decoder, TraceableValue}](secretId: SecretIdType): F[A] } +@annotation.experimental object SecretsManagerAlg { - given Aspect[SecretsManagerAlg, TraceableValue, TraceableValue] = new Aspect[SecretsManagerAlg, TraceableValue, TraceableValue] { - override def weave[F[_]](af: SecretsManagerAlg[F]): SecretsManagerAlg[Aspect.Weave[F, TraceableValue, TraceableValue, *]] = - new SecretsManagerAlg[Aspect.Weave[F, TraceableValue, TraceableValue, *]] { - override def getSecret(secretId: SecretIdType): Aspect.Weave[F, TraceableValue, TraceableValue, Secret] = - Aspect.Weave( - "SecretsManagerAlg", - List(List( - Aspect.Advice.byValue("secretId", secretId), - )), - Aspect.Advice("getSecret", af.getSecret(secretId)) - ) + given Aspect[SecretsManagerAlg, TraceableValue, TraceableValue] = Derive.aspect - override def getSecretAs[A: Decoder](secretId: SecretIdType): Aspect.Weave[F, TraceableValue, TraceableValue, A] = - Aspect.Weave( - "SecretsManagerAlg", - List( - List(Aspect.Advice.byValue("secretId", secretId)), - List(Aspect.Advice.byValue("implicit decoder", Decoder[A].toString)), - ), - Aspect.Advice("getSecretAs", af.getSecretAs[A](secretId))(using TraceableValue[String].contramap[A](_ => "redacted successfully parsed and decoded secret")) - ) - } - - override def mapK[F[_], G[_]](af: SecretsManagerAlg[F])(fk: F ~> G): SecretsManagerAlg[G] = - new SecretsManagerAlg[G] { - override def getSecret(secretId: SecretIdType): G[Secret] = fk(af.getSecret(secretId)) - override def getSecretAs[A: Decoder](secretId: SecretIdType): G[A] = fk(af.getSecretAs[A](secretId)) - } - } - - def apply[F[_] : Async : Logger : Trace](client: SecretsManager[F]): SecretsManagerAlg[F] = + def apply[F[_] : {Async, Logger, Trace}](client: SecretsManager[F]): SecretsManagerAlg[F] = WeaveKnot[SecretsManagerAlg, F](apply(client, _))(_.traceWithInputsAndOutputs) - private def apply[F[_] : Async : Logger](client: SecretsManager[F], - self: Eval[SecretsManagerAlg[F]], - ): SecretsManagerAlg[F] = new SecretsManagerAlg[F] { + private def apply[F[_] : {Async, Logger}](client: SecretsManager[F], + self: Eval[SecretsManagerAlg[F]], + ): SecretsManagerAlg[F] = new SecretsManagerAlg[F] { private val parser = new JawnParser override def getSecret(secretId: SecretIdType): F[Secret] = @@ -71,7 +45,7 @@ object SecretsManagerAlg { NoSecretInResponseException(secretId).raiseError[F, Secret] } - override def getSecretAs[A: Decoder](secretId: SecretIdType): F[A] = + override def getSecretAs[A : {Decoder, TraceableValue}](secretId: SecretIdType): F[A] = self.value.getSecret(secretId) .flatMap { case SecretString(SecretStringType(value)) => parser.parse(value).liftTo[F] diff --git a/src/main/scala/com/dwolla/postgres/init/repositories/CreateSkunkSession.scala b/src/main/scala/com/dwolla/postgres/init/repositories/CreateSkunkSession.scala index 823e632..a180c0b 100644 --- a/src/main/scala/com/dwolla/postgres/init/repositories/CreateSkunkSession.scala +++ b/src/main/scala/com/dwolla/postgres/init/repositories/CreateSkunkSession.scala @@ -41,7 +41,7 @@ object CreateSkunkSession { username: MasterDatabaseUsername, password: MasterDatabasePassword, ) - (using CreateSkunkSession[F], MonadCancelThrow[F]): F[A] = + (using CreateSkunkSession[F], MonadCancelThrow[F], Trace[F]): F[A] = impl(host, port, username, password, none) def inSession(host: Host, @@ -50,24 +50,27 @@ object CreateSkunkSession { password: MasterDatabasePassword, database: Database, ) - (using CreateSkunkSession[F], MonadCancelThrow[F]): F[A] = + (using CreateSkunkSession[F], MonadCancelThrow[F], Trace[F]): F[A] = impl(host, port, username, password, database.some) private def impl(host: Host, - port: Port, - username: MasterDatabaseUsername, - password: MasterDatabasePassword, - database: Option[Database], - ) - (using CreateSkunkSession[F], MonadCancelThrow[F]): F[A] = - CreateSkunkSession[F].single( - host = host.show, - port = port.value, - user = username.value.value, - database = database.map(_.value).getOrElse(sqlIdentifier"postgres").value, - password = password.value.some, - ssl = if (host == host"localhost") SSL.None else SSL.System, - ).use(kleisli.run) + port: Port, + username: MasterDatabaseUsername, + password: MasterDatabasePassword, + database: Option[Database], + ) + (using CreateSkunkSession[F], MonadCancelThrow[F], Trace[F]): F[A] = + Trace[F].span("database.session") { + Trace[F].put("database" -> database.map(_.value).getOrElse(sqlIdentifier"postgres").value) >> + CreateSkunkSession[F].single( + host = host.show, + port = port.value, + user = username.value.value, + database = database.map(_.value).getOrElse(sqlIdentifier"postgres").value, + password = password.value.some, + ssl = if (host == host"localhost") SSL.None else SSL.System, + ).use(kleisli.run) + } extension [F[_], A](fa: F[A]) def recoverUndefinedAs(a: A) From 62e2a14152bcd9862f39eebb0b0c95fc87f2f4ee Mon Sep 17 00:00:00 2001 From: Brian Holt Date: Mon, 17 Nov 2025 21:38:32 -0600 Subject: [PATCH 2/3] remove defunct users when updating a database --- .../PostgresqlDatabaseInitHandlerImpl.scala | 2 ++ .../init/repositories/UserRepository.scala | 33 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/main/scala/com/dwolla/postgres/init/PostgresqlDatabaseInitHandlerImpl.scala b/src/main/scala/com/dwolla/postgres/init/PostgresqlDatabaseInitHandlerImpl.scala index 13c2860..decf4f8 100644 --- a/src/main/scala/com/dwolla/postgres/init/PostgresqlDatabaseInitHandlerImpl.scala +++ b/src/main/scala/com/dwolla/postgres/init/PostgresqlDatabaseInitHandlerImpl.scala @@ -52,6 +52,8 @@ object PostgresqlDatabaseInitHandlerImpl { _ <- userPasswords.traverse { userPassword => userRepository.addOrUpdateUser(userPassword) >> roleRepository.addUserToRole(userPassword.user, userPassword.database) } + usersToRemove <- userRepository.findDefunctUsers(userPasswords.map(_.user), RoleRepository.roleNameForDatabase(input.name)) + _ <- usersToRemove.traverse_(userRepository.removeUser) } yield () private def createOrUpdate(userPasswords: List[UserConnectionInfo], input: DatabaseMetadata): F[PhysicalResourceId] = diff --git a/src/main/scala/com/dwolla/postgres/init/repositories/UserRepository.scala b/src/main/scala/com/dwolla/postgres/init/repositories/UserRepository.scala index dd7e376..eb4e9b9 100644 --- a/src/main/scala/com/dwolla/postgres/init/repositories/UserRepository.scala +++ b/src/main/scala/com/dwolla/postgres/init/repositories/UserRepository.scala @@ -21,11 +21,16 @@ import scala.concurrent.duration.* trait UserRepository[F[_]] { def addOrUpdateUser(userConnectionInfo: UserConnectionInfo): F[Username] def removeUser(username: Username): F[Username] + def findDefunctUsers(usersThatShouldExist: List[Username], role: RoleName): F[List[Username]] } @annotation.experimental object UserRepository { - given Aspect[UserRepository, TraceableValue, TraceableValue] = Derive.aspect + given Aspect[UserRepository, TraceableValue, TraceableValue] = { + given TraceableValue[List[Username]] = TraceableValue[String].contramap(_.mkString("[", ", ", "]")) + + Derive.aspect + } def usernameForDatabase(database: Database): Username = Username(database.value) @@ -82,6 +87,13 @@ object UserRepository { override def removeUser(username: Username): Kleisli[F, Session[F], Username] = removeUser(username, 5) + + override def findDefunctUsers(usersThatShouldExist: List[Username], + roleName: RoleName, + ): Kleisli[F, Session[F], List[Username]] = + Kleisli[F, Session[F], List[Username]] { + _.execute(UserQueries.findDefunctUsers(usersThatShouldExist))(usersThatShouldExist *: roleName *: EmptyTuple) + } }.traceWithInputsAndOutputs } @@ -89,6 +101,9 @@ object UserQueries { private val username: skunk.Codec[Username] = name.eimap[Username](refineV[SqlIdentifierPredicate](_).map(Username(_)))(_.value.value) + private val roleName: skunk.Codec[RoleName] = + name.eimap[RoleName](refineV[SqlIdentifierPredicate](_).map(RoleName(_)))(_.value.value) + val checkUserExists: Query[Username, Username] = sql"SELECT u.usename FROM pg_catalog.pg_user u WHERE u.usename = $username" .query(username) @@ -106,4 +121,20 @@ object UserQueries { def removeUser(username: Username): Command[Void] = sql"DROP USER IF EXISTS #${username.value.value}" .command + + def findDefunctUsers(users: List[Username]): Query[users.type *: RoleName *: EmptyTuple, Username] = + sql""" + WITH expected(role_name) AS (VALUES (${username.list(users)})) + SELECT + m.rolname AS user_name + FROM pg_auth_members rm + JOIN pg_roles r ON r.oid = rm.roleid + JOIN pg_roles m ON m.oid = rm.member + WHERE r.rolname = $roleName + AND m.rolcanlogin = TRUE + AND m.rolname NOT IN (SELECT role_name FROM expected) + ORDER BY user_name; + """ + .query(username) + } From 4a36c22ea97bd67c3c0b8f81780261ad3c739a04 Mon Sep 17 00:00:00 2001 From: Brian Holt Date: Mon, 17 Nov 2025 21:38:52 -0600 Subject: [PATCH 3/3] add Junie guidelines --- .junie/guidelines.md | 92 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 .junie/guidelines.md diff --git a/.junie/guidelines.md b/.junie/guidelines.md new file mode 100644 index 0000000..785e233 --- /dev/null +++ b/.junie/guidelines.md @@ -0,0 +1,92 @@ +### Project-specific development guidelines + +This document summarizes non-obvious, project-specific details to help advanced contributors build, test, and debug this repo reliably. + +#### Build and configuration +- Toolchain + - Scala 3.7.4 (see `build.sbt`), JVM 17 (GitHub CI uses Temurin 17). Use Java 17 locally to match. + - Build tool: sbt (standard layout). Packaging uses `sbt-native-packager` (`JavaAppPackaging` and `UniversalPlugin`). + - Codegen: The `smithy` subproject is enabled with `com.disneystreaming.smithy4s` codegen. sbt will generate code for Smithy models before compiling dependents. No manual steps are needed beyond running sbt. + +- Subprojects + - Root project is named `postgresql-init-core` in `build.sbt` and depends on `:smithy` for generated Smithy4s code. + - Smithy models live under `smithy/src/main`. + +- Dependencies and noteworthy versions + - Skunk 0.6.4 for PostgreSQL access. + - Cats Effect 3 ecosystem, Natchez for tracing (including X-Ray), `log4cats` + `log4j2` for logging. + - Test stack: MUnit 1.2.x, `munit-scalacheck`, and `discipline-munit`. + +- Serverless packaging and deploy (optional, for AWS publication) + - `serverless.yml` is configured to deploy two Lambda artifacts. `sbt deploy` shells out to `serverless deploy`. + - Required environment for deployment (see README): `BUCKET`, `ACCOUNT`, `STAGE`, `SUBNET_ID`, `SECURITY_GROUP`. + - sbt will pass `DATABASE_ARTIFACT_PATH` to Serverless at deploy-time. + - Local deploy command string is configurable via `serverlessDeployCommand` in `build.sbt`. + +- Local manual invocation for development + - `src/test/scala/com/dwolla/postgres/init/LocalApp.scala` contains a small `IOApp` for running the handler locally against a real Postgres instance and real AWS Secrets Manager. This is for integration/manual testing; it requires valid AWS credentials, an accessible DB, and OTel environment if you want traces. + +- Network/SSL conventions for DB connections + - `CreateSkunkSession` chooses `SSL.None` for `localhost` and `SSL.System` otherwise, and picks `postgres` as the default database unless a specific `Database` is provided. This is relevant when simulating local connections. + +#### Testing: how to run, add, and scope tests +- Running all tests + - From project root: `sbt test`. This builds the Smithy subproject if needed, compiles tests, and runs all MUnit suites under `src/test/scala`. + +- Running a single suite or test + - Single suite: `sbt "testOnly com.dwolla.postgres.init.ExtractRequestPropertiesSpec"` + - Single test within a suite: `sbt "testOnly com.dwolla.postgres.init.SqlStringRegexSpec -- *passwords*"` (MUnit supports test filters via `--` followed by a glob pattern matching the test name.) + +- Property-based and law checks + - The project uses ScalaCheck and Discipline. Example: `SqlStringRegexSpec` combines Arbitrary instances with `checkAll` law tests (`SemigroupTests[SqlIdentifier]`). If you add new `cats` typeclass instances, consider adding law checks here. + +- Example: adding and running a simple test (demonstrated and verified) + - A trivial MUnit suite named `DemoSanitySpec` was added temporarily to verify commands: + ```scala + package com.dwolla.postgres.init + + class DemoSanitySpec extends munit.FunSuite { + test("demo sanity: 2 + 2 == 4") { + assertEquals(2 + 2, 4) + } + } + ``` + - Run it directly: `sbt "testOnly com.dwolla.postgres.init.DemoSanitySpec"`. + - It passed locally; the file has been removed to keep the repo clean. + +- Adding new tests + - Place suites under `src/test/scala` using package `com.dwolla.postgres.init` (match sources if testing package-private APIs). + - Use `munit.FunSuite` for example-based tests, and add `munit-scalacheck` for generators/properties. For typeclass laws, add `DisciplineSuite` + `discipline-munit`. + - Prefer deterministic unit tests. For DB integration, use `LocalApp` or an ephemeral containerized Postgres with fixed schema/fixtures. Avoid hitting real AWS in automated tests. + +- Test selection pitfalls + - The test runner in this repo is MUnit; use `testOnly` with fully qualified suite names. Running a directory path (e.g., `testOnly src/test/scala`) will not work—use FQCN instead. + +#### Development conventions and useful internals +- Functional style and effects + - Prefer tagless-final and `cats` typeclasses. Constructors are expressed via givens/context bounds where practical (e.g., `given [F[_] : {Temporal, Trace, Network, Console}]: CreateSkunkSession[F] = Session.single`). + - Encourage `Resource` + `Kleisli` patterns for DB sessions. See `CreateSkunkSession` extension methods like `inSession` and `recoverUndefinedAs` for ergonomics. + +- SQL identifiers and input validation + - See `SqlStringRegexSpec` for refined predicates: SQL identifiers follow `[A-Za-z][A-Za-z0-9_]*`, and generated passwords exclude dangerous punctuation. When adding features that touch SQL strings, validate via the existing newtypes/refinements to maintain safety. + +- Tracing & logging + - Natchez is integrated. Use `Trace[F]` in new effectful APIs where traces are relevant. For local runs, `LocalApp` wires `OpenTelemetryAtDwolla` and logs via SLF4J/Log4j2. + +- Build info and runtime metadata + - `BuildInfoPlugin` is enabled. Access via `com.dwolla.buildinfo.postgres.init.BuildInfo` (used in `LocalApp`) for name/version/etc. Keep it in sync when adding new runtime info. + +- CI nuances + - CI config is under `.dwollaci.yml` and GitHub Actions. CI uses JDK 17 and runs `sbt test`. + - If you adjust the plugin or Java version, update CI accordingly. Use `sbt githubWorkflowGenerate` to regenerate the GitHub Actions workflow YAML. + +#### Known quirks and fixes +- `build.sbt` defines `serverlessDeployCommand` as `Seq[String]`. On some Scala 3 toolchains you may need an explicit `immutable.Seq` to satisfy `settingKey[Seq[String]]`. If you see a type mismatch like `found: scala.collection.Seq[String]`, change to: `serverlessDeployCommand := scala.collection.immutable.Seq.from("serverless deploy --verbose".split(' '))` or similar. +- README JSON blocks are illustrative and may not be strict JSON; treat them as examples for CloudFormation. + +#### Quickstart commands (verified) +- Full build + tests: `sbt test` +- One suite: `sbt "testOnly com.dwolla.postgres.init.ExtractRequestPropertiesSpec"` +- One test in a suite: `sbt "testOnly com.dwolla.postgres.init.SqlStringRegexSpec -- *passwords*"` +- Package universal distribution: `sbt Universal/packageBin` +- Deploy (requires env vars and Serverless): `sbt deploy`