Implement feature booking by ducls-2223 · Pull Request #3 · awesome-academy/duc_java

ducls-2223 · 2026-07-27T08:44:51Z

No description provided.

chienpv-3590

PR chất lượng tốt: hexagonal architecture tách bạch, userId lấy từ JWT chứ không tin client, totalPrice tính ở server, hai thao tác tranh chấp (giữ chỗ và hủy đơn) đều dùng atomic UPDATE thay vì check-then-act, và test coverage cho các service khá đầy đủ. Các comment giải thích "tại sao" trong code cũng đáng khen.

Có 2 blocker cần xử lý trước khi merge, cả hai xoay quanh idempotency key. (1) CreateBookingService dòng 36: booking tìm theo idempotency key được trả thẳng về mà không kiểm tra người tra cứu có phải chủ đơn không, cộng với unique index toàn cục ở V16 dòng 1 thì user B chỉ cần trùng key với user A là đọc được đơn của A kèm contactEmail/contactPhone — đây là IDOR, không phải trường hợp hi hữu. (2) Cùng lúc đó V16 dòng 1 có ADD COLUMN ... NOT NULL không DEFAULT, sẽ fail nếu bảng đã có dữ liệu. Cách sửa gọn cho cả hai: đổi ràng buộc thành UNIQUE (user_id, idempotency_key) và tra cứu theo cặp (userId, idempotencyKey).

Ba điểm nên cân nhắc: CreateBookingService dòng 71 — hai request đồng thời cùng key sẽ cho 500 thay vì trả về đơn đã tạo, tức là đúng kịch bản mà idempotency sinh ra để chống; BookingController dòng 160 — status filter sai chính tả bị nuốt và trả về toàn bộ đơn; GlobalExceptionHandler dòng 95 — đổi 400 sang 422 là breaking change cho mọi endpoint, cần ghi vào mô tả PR và rà lại @ApiResponse ở các controller khác. Còn lại là nit ở ContactInfo dòng 9, BookingPersistenceAdapter dòng 52 và CancelBookingService dòng 35.

Về test: bộ test hiện có tốt nhưng thiếu đúng ca của blocker — nên thêm một test cho CreateBookingService khẳng định request của user B với key đã dùng bởi user A không nhận được đơn của A. BookingPersistenceAdapter (logic sinh bookingCode) cũng chưa có test nào.

chienpv-3590 · 2026-07-27T09:08:43Z

+        // returns the already-created booking instead of reserving slots a second time.
+        Optional<Booking> existing = bookingRepository.findByIdempotencyKey(command.idempotencyKey());
+        if (existing.isPresent()) {
+            return existing.get();


Blocker — rò rỉ đơn hàng giữa các user (IDOR). findByIdempotencyKey đang tra cứu toàn cục, không gắn với command.userId(), và booking tìm được được trả thẳng về mà không kiểm tra chủ sở hữu. Key này do client tự sinh và chỉ bị ràng buộc @NotBlank, nên chỉ cần user B gửi một key trùng với user A (vd "1", "booking-1") là nhận nguyên đơn của A: bookingCode, totalPrice, contactName/contactEmail/contactPhone. Unique index toàn cục ở V16 làm điều này thành hành vi chắc chắn xảy ra chứ không phải hi hữu. Nên đổi sang tra cứu theo cặp (userId, idempotencyKey):

Optional<Booking> existing = bookingRepository .findByUserIdAndIdempotencyKey(command.userId(), command.idempotencyKey()); if (existing.isPresent()) { return existing.get(); }

chienpv-3590 · 2026-07-27T09:10:10Z

@@ -0,0 +1 @@
+ALTER TABLE bookings ADD COLUMN idempotency_key VARCHAR(100) NOT NULL UNIQUE;


Blocker — hai vấn đề trên cùng một dòng. (1) UNIQUE toàn cục nghĩa là không gian key được chia sẻ giữa mọi user, đây chính là gốc của lỗ hổng ở comment dòng 36 CreateBookingService.java; idempotency key nên chỉ duy nhất trong phạm vi một user. (2) ADD COLUMN ... NOT NULL không có DEFAULT sẽ fail ngay nếu bảng bookings đã có dữ liệu — hiện an toàn vì V15 nằm cùng PR, nhưng nếu V15 đã lên môi trường nào trước đó thì migration này chết. Gộp cột vào luôn V15 hoặc viết lại:

ALTER TABLE bookings ADD COLUMN idempotency_key VARCHAR(100); UPDATE bookings SET idempotency_key = 'legacy-' || id WHERE idempotency_key IS NULL; ALTER TABLE bookings ALTER COLUMN idempotency_key SET NOT NULL; ALTER TABLE bookings ADD CONSTRAINT uq_bookings_user_idem UNIQUE (user_id, idempotency_key);

chienpv-3590 · 2026-07-27T09:11:31Z

+                command.contactPhone()
+        );
+
+        return bookingRepository.save(booking);


Consideration — race condition làm hỏng chính cơ chế idempotency. Hai request cùng key gửi đồng thời (đúng kịch bản double-submit mà key này sinh ra để chống) đều thấy Optional.empty() ở dòng 34, cả hai cùng đi tiếp, và request thua sẽ chết ở unique constraint tại đây → DataIntegrityViolationException. GlobalExceptionHandler hiện không bắt exception này nên client nhận 500 thay vì được trả về đơn đã tạo. Slots thì rollback đúng nhờ @Transactional, nhưng trải nghiệm vẫn sai. Bắt và đọc lại theo key:

try { return bookingRepository.save(booking); } catch (DataIntegrityViolationException ex) { return bookingRepository .findByUserIdAndIdempotencyKey(command.userId(), command.idempotencyKey()) .orElseThrow(() -> ex); }

chienpv-3590 · 2026-07-27T09:12:17Z

+        try {
+            return BookingStatus.valueOf(status.trim().toUpperCase());
+        } catch (IllegalArgumentException ex) {
+            return null;


Consideration — nuốt lỗi filter dễ gây hiểu nhầm. status sai chính tả (vd ?status=pendign) sẽ rơi vào đây, trả về null và endpoint trả toàn bộ đơn của user như thể không có filter. Client không có cách nào biết filter đã bị bỏ qua, và với UI phân trang/thống kê thì đây là kết quả sai chứ không phải kết quả rỗng. Hành vi này có được note trong @Operation nên là chủ ý, nhưng cân nhắc trả 422 cho giá trị không hợp lệ để nhất quán với cách PR đang xử lý input sai ở chỗ khác:

} catch (IllegalArgumentException ex) { throw new InvalidBookingStatusFilterException(status); }

chienpv-3590 · 2026-07-27T09:12:55Z

                .map(fieldError -> fieldError.getField() + ": " + fieldError.getDefaultMessage())
                .collect(Collectors.joining("; "));
-        return build(HttpStatus.BAD_REQUEST, message, request);
+        return build(HttpStatus.UNPROCESSABLE_CONTENT, message, request);


Consideration — breaking change toàn API, không chỉ Bookings. Handler này áp cho mọi controller, nên mọi endpoint đang trả 400 khi validate body sẽ đổi sang 422 sau PR này. Hai điểm cần xử lý trước khi merge: (1) client hiện tại (web/mobile) nếu đang branch theo status === 400 sẽ vỡ — nên là một dòng trong mô tả PR/CHANGELOG chứ không nằm im trong một feature PR về booking; (2) PR mới sửa @ApiResponse của AuthController.register, các controller khác đang document 400 cho lỗi validate body giờ đã sai so với thực tế — nên rà soát và sửa đồng loạt. Lý do chọn 422 thì hợp lý và comment giải thích ở trên rất tốt.

chienpv-3590 · 2026-07-27T09:13:43Z

+
+public record ContactInfo(
+        @NotBlank(message = "contact.name không được để trống")
+        String name,


Nit — thiếu ràng buộc độ dài, lệch với schema DB. contact_name và contact_email là VARCHAR(255) trong V15 nhưng ở đây không có @Size, nên một name dài 300 ký tự sẽ qua được validation rồi chết ở tầng DB → 500 thay vì 422 với message rõ ràng. phone thì đã an toàn nhờ regex {8,20} khớp đúng VARCHAR(20). Thêm cho hai field còn lại:

@NotBlank(message = "contact.name không được để trống") @Size(max = 255, message = "contact.name tối đa 255 ký tự") String name,

chienpv-3590 · 2026-07-27T09:14:25Z

+        // IDENTITY generation inserts eagerly, so the id is populated right after this call.
+        BookingEntity inserted = bookingJpaRepository.save(entity);
+        inserted.setBookingCode(generateBookingCode(inserted.getId(), inserted.getCreatedAt()));
+        BookingEntity saved = bookingJpaRepository.save(inserted);


Nit — mỗi booking tốn 1 INSERT + 1 UPDATE chỉ để sinh bookingCode. Comment ở trên giải thích lý do rất rõ và cách này đúng về mặt correctness, nhưng UUID placeholder ghi vào một cột UNIQUE rồi ghi đè ngay sau đó là một vòng ghi thừa trên đường đi nóng nhất của feature. Nếu muốn bỏ lượt UPDATE: dùng một Postgres sequence riêng cho mã đơn và lấy nextval trước khi build entity, hoặc để DB tự sinh bằng cột generated. Không chặn merge, chỉ là chỗ đáng dọn khi có dịp.

Một chi tiết nhỏ nữa: createdAt được set bằng OffsetDateTime.now() ở dòng 46 trong khi V15 đã có DEFAULT now() — hai nguồn sự thật cho cùng một giá trị, nên bỏ một trong hai.

chienpv-3590 · 2026-07-27T09:15:11Z

+        // flip PENDING/CONFIRMED -> CANCELLED; a losing request gets false here instead of both
+        // passing a stale in-memory status check and double-releasing slots.
+        if (!bookingRepository.cancelIfCancellable(bookingId)) {
+            throw new BookingCancellationNotAllowedException(booking.status());


Nit — message lỗi dùng status đã cũ. Atomic guard ở dòng 34 là hoàn toàn đúng, nhưng booking.status() ở đây là snapshot đọc từ trước khi cancelIfCancellable chạy. Đúng ở case "đơn vốn đã CANCELLED/COMPLETED"; sai ở case thua race — request thua sẽ báo Không thể hủy đơn ở trạng thái: PENDING, đọc lên như một bug của server. Đọc lại status thật rồi mới dựng message:

if (!bookingRepository.cancelIfCancellable(bookingId)) { BookingStatus current = bookingRepository.findById(bookingId) .map(Booking::status) .orElseThrow(() -> new BookingNotFoundException(bookingId)); throw new BookingCancellationNotAllowedException(current); }

ducls-2223 added 2 commits July 27, 2026 15:00

implement api POST /bookings

e909f4f

implement api GET /bookings

bb0761f

ducls-2223 changed the title ~~Feat/bookings~~ Implement feature booking Jul 27, 2026

chienpv-3590 reviewed Jul 27, 2026

View reviewed changes

refactor code, fix comment

fac797a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement feature booking - #3

Implement feature booking#3
ducls-2223 wants to merge 3 commits into
awesome-academy:masterfrom
ducls-2223:feat/bookings

ducls-2223 commented Jul 27, 2026

Uh oh!

chienpv-3590 left a comment

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

chienpv-3590 Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1 @@
		ALTER TABLE bookings ADD COLUMN idempotency_key VARCHAR(100) NOT NULL UNIQUE;

Uh oh!

Conversation

ducls-2223 commented Jul 27, 2026

Uh oh!

chienpv-3590 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants