Live coding · Code review · Güvenlik · Performans

Bu PR'ı inceleyip yorum yaz

OrtaHedef 15 dk

Glassdoor: "How do you review a pull request?" sorusu

00:00/ 15:00 hedefÖnce kendin çöz; ipuçlarını sırayla aç.

Bir takım arkadaşın aşağıdaki PR'ı açtı ve "küçük bir değişiklik, hızlıca onaylar mısın?" dedi. Mülakatçı: "Ekranı paylaş, PR'ı birlikte inceleyelim. Hangi yorumları yazarsın, hangileri merge'ü engeller?"

Görevler

Bulduğun her problemi yaz: ne, neden önemli, nasıl düzeltilir.
Her yorumu ciddiyetine göre işaretle: Engelleyici (blocker) / Önemli / Öneri (nit).
En az 6 problem var.

Başlangıç kodu

@RestController
public class CustomerController {

    @Autowired
    private JdbcTemplate jdbcTemplate;

    @Autowired
    private OrderRepository orderRepository;

    @GetMapping("/customers/search")
    public List<Map<String, Object>> search(@RequestParam String name) {
        return jdbcTemplate.queryForList("SELECT * FROM customers WHERE name LIKE '%" + name + "%'");
    }

    @GetMapping("/customers/{id}/orders")
    public List<Order> orders(@PathVariable Long id) {
        List<Order> orders = orderRepository.findByCustomerId(id);
        for (Order o : orders) {
            o.getItems().size();   // lazy yükleme
        }
        log.info("Orders listed for {} email={} card={}",
                id, orders.get(0).getCustomer().getEmail(), orders.get(0).getPayment().getCardNumber());
        return orders;
    }

    @PostMapping("/customers/{id}/points")
    public void addPoints(@PathVariable Long id, @RequestParam int points) {
        updatePoints(id, points);
    }

    @Transactional
    private void updatePoints(Long id, int points) {
        Customer c = customerRepository.findById(id).get();
        c.setPoints(c.getPoints() + points);
        customerRepository.save(c);
    }
}

// Yorumlarını buraya yaz:

İpucu 1

Önce güvenlik: string birleştirmeyle kurulan SQL, log'a yazılan veriler, kimin hangi müşterinin puanını değiştirebildiği.

İpucu 2

Sonra doğruluk: boş liste, Optional.get(), private + @Transactional, eşzamanlı puan ekleme.

İpucu 3

Sonra performans ve tasarım: döngüdeki lazy yükleme, sayfalama, entity'nin dışarı açılması, controller'daki iş mantığı, field injection, REST isimlendirme.

Referans çözümü göster

Çözüm

[BLOCKER] SQL injection — search()
  "name" doğrudan SQL'e ekleniyor: name = "%' OR '1'='1" tüm tabloyu döker, "'; DROP TABLE ..." daha kötüsü.
  → jdbcTemplate.queryForList("SELECT id, name FROM customers WHERE name ILIKE ?", "%" + name + "%")
    ya da Spring Data: findByNameContainingIgnoreCase(name, pageable). Sayfalama ve kolon seçimi de ekle.

[BLOCKER] Kart numarası ve e-posta log'lanıyor — orders()
  Kart numarası log'a yazılamaz (PCI DSS); e-posta kişisel veri (KVKK). Log'lar geniş erişimlidir ve uzun saklanır.
  → Sadece customerId ve sipariş sayısı. Kart numarası zaten tutulmamalı (tokenization).

[BLOCKER] Yetkilendirme yok — /customers/{id}/orders ve /points
  Kimliği doğrulanmış her kullanıcı başka bir müşterinin id'siyle siparişlerini okuyabilir ve puan ekleyebilir (IDOR).
  → Müşteri kendi id'si mi (SecurityContext), puan ekleme sadece iç servis/admin rolü.

[BLOCKER] @Transactional private metotta + self-invocation
  Proxy private metodu ezemez ve this üzerinden çağrı proxy'den geçmez; transaction yok.
  → Mantığı bir LoyaltyService'in public @Transactional metoduna taşı.

[ÖNEMLİ] Lost update — updatePoints()
  İki eşzamanlı istek aynı puanı okuyup üzerine yazar.
  → UPDATE customers SET points = points + :p WHERE id = :id (atomik) ya da @Version + retry.

[ÖNEMLİ] orders.get(0) boş listede IndexOutOfBoundsException → 500.
[ÖNEMLİ] findById(id).get() → NoSuchElementException → 500. orElseThrow(CustomerNotFoundException) → 404.

[ÖNEMLİ] N+1 + entity dönmek — orders()
  Döngüde getItems() her sipariş için ayrı sorgu. Entity'yi doğrudan dönmek lazy alanları JSON'da patlatır
  ve iç alanları (payment!) dışarı sızdırır.
  → DTO projection ya da @EntityGraph("items") + Pageable; OrderSummaryResponse dön.

[ÖNEMLİ] Validasyon: points negatif ya da çok büyük olabilir → @Positive @Max(...).
  POST void döndürüyor: 204 No Content ya da güncel puanı dön.

[ÖNERİ] customerRepository tanımlı değil (derlenmez); PR CI'dan geçmemiş olmalı.
[ÖNERİ] Field injection → constructor injection (final alanlar, test edilebilirlik).
[ÖNERİ] İş mantığı controller'da → servis katmanı. URL'ler /api/v1/customers/...
[ÖNERİ] PR açıklaması ve test yok: "Bu değişiklikle hangi test eklendi?"

Açıklama: neden böyle?

İyi bir PR review'ün sırası:

Güvenlik
Doğruluk ve veri bütünlüğü
Performans
Tasarım ve okunurluk
Stil (nit)

Engelleyici olanları açıkça ayır. Stil yorumlarını linter ve formatter'a bırak, insan dikkatini önemli şeylere harca.

Yorum dili: Kişiyi değil kodu eleştir. "Bunu neden böyle yaptın?" yerine "Burada X riski var; Y önerebilir miyim?" Öneriyle gel, kodla örnek ver.
"İyi commit" sorusu:
- Tek ve atomik bir değişiklik.
- Derlenir ve testleri geçer.
- Mesaj nedeni anlatır.
- Conventional Commits formatı (fix: prevent SQL injection in customer search).
- Refactor ile davranış değişikliği ayrı commit'lerde.
Küçük PR: 400+ satırlık PR'lar iyi incelenemez. Büyük işleri feature flag arkasında küçük PR'lara böl.

SQL injection: string birleştirme → parametreli sorgu (blocker)
Kişisel veri ve kart numarasının log'lanması (KVKK/PCI DSS) (blocker)
Yetkilendirme yok: herkes herkese puan ekleyebilir veya başkasının siparişini görebilir (IDOR)
orders.get(0) boş listede IndexOutOfBounds; Optional.get() → NoSuchElement/500
@Transactional private metotta ve self-invocation → çalışmaz
Lost update: oku-değiştir-yaz yarışı → atomik UPDATE / locking
N+1 (döngüde lazy yükleme) ve sayfalama yok
Entity'nin/Map'in doğrudan dönülmesi, SELECT *; DTO kullanılmalı
Tasarım: iş mantığı controller'da, field injection, customerRepository tanımsız, /api/v1 yok, negatif puan validasyonu

Bu PR'ın yazarı junior ve moral olarak hassas. Yorumlarını nasıl yazarsın?
Hangi kontroller insan yerine CI'da otomatik yapılmalı? (SAST, dependency scan, ArchUnit, formatter)
Senin PR'ına katılmadığın bir yorum gelirse ne yaparsın?