fix: code review improvements across delivery, usecase and consumer l…#46
Open
dkoshenkov wants to merge 2 commits into
Open
fix: code review improvements across delivery, usecase and consumer l…#46dkoshenkov wants to merge 2 commits into
dkoshenkov wants to merge 2 commits into
Conversation
…ayers - bots search: ILIKE was matching exactly because the term was passed without wildcards. Wrap the (LIKE-escaped) term in %...% and add ESCAPE '\' so search performs a real substring match and user-supplied %/_ are matched literally. - error mapping: replace fragile strings.Contains(err.Error(), ...) checks with errors.Is against sentinel errors in posts_query handlers; also fix a fall-through that returned success on unmapped errors and a 500 that was wrongly encoded as a NotFound response. - stop leaking internal error text to clients: all 5xx (InternalServerError / ServiceUnavailable) responses now return a generic message and log the real error; 4xx validation messages are preserved. - kafka consumer: poison (undecodable) messages and unknown commands now send an error reply to the request-reply caller and commit instead of being silently dropped or blocking the partition forever. - bots create: drop the redundant re-read of the just-created bot by adding Service.CreateWithProfiles, which resolves profiles without a second Get. - add unit tests for the LIKE escaper and the bots create/CreateWithProfiles usecase logic.
… handling Security / correctness (high): - IDOR: post update/delete/seen/publish never set UserID, and the command repos filtered only by id — any user could mutate any post. Thread UserID from the producer handlers through the Kafka DTOs into the repos, which now scope by `AND user_id = $N`. - profiles.GetByID was not tenant-scoped (cross-tenant read by ID); now filters by user_id. The gRPC-only GetByIDs/Exist stay by-ID (no user_id in contract) and are documented as such. - repo CreatePost: INSERT omitted NOT-NULL bot_name/profile_name and user_id (every insert failed); the categories branch returned before tx.Commit (silently dropping the write); and the deferred rollback overwrote the real error. All three fixed. - auth.Config expected `addr` but every config supplies auth_grpc.host/port, so the dial address was always empty and auth always failed. Config now reads host/port (addr still honored as an override) and errors if unset. - posts_query Run() built a second auth client with a nil logger (panicked on the first auth failure); it now reuses the client from the app struct. - bots handlers returned (nil, err), yielding a generic 500 for validation (should be 400) and not-found (should be 404); they now return typed ogen responses. SeenPosts not-found returned a 500-typed body; now 404. Lifecycle / robustness (medium): - kafka request-reply: a double Ack/Nack/Reply could block the handler on a full channel; the decision signal is now non-blocking and documented. - kafka basic consumer: `defer cancel()` inside the read loop accumulated one live timer per iteration; now cancels per iteration. - graceful shutdown: consumer/producer mains handle SIGINT/SIGTERM and call Close() instead of relying on a defer that never runs; Close errors are logged rather than log.Fatal'd inside a defer (which skipped remaining cleanup). - aggregator/reddit mains: handle the previously-ignored config load error and drop the no-op WatchConfig reassignment (constructor had already captured the pointer); reddit no longer log.Fatal's from the watch callback. - aggregator AddTopics batch was never Closed (leak); reddit performRequests leaked the response body on 403, could block goroutines on unbuffered sends, and cancelled its context on return while feeding goroutines still ran. Low: - profiles.GetByID/bots.Get use errors.Is / RowToStructByName; bots Update and Add/RemoveProfileID now filter is_deleted = false. - logger Warn/Info/Debug joined all supplied errors instead of dropping them when more than one was passed. Tests: add unit tests for auth.dialAddr, logger.joinErrs, and the KafkaUpdatePostRequest UserID mapping (the IDOR guard).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ayers