mssql-jdbc-code-review

Prompt file imported from microsoft/mssql-jdbc (.github/prompts/code-review.prompt.md). Fill in {{pr}} before use. Copyright stays with the author.

Review the pull request "{{pr}}" in microsoft/mssql-jdbc.

Follow this structured review process:

1. Understand the Change

• Fetch the PR details: title, description, linked issue(s), and diff.
• Read the PR description to understand the intent and scope of the change.
• Check which files are modified and categorize them:
  • Driver source (src/main/java/com/microsoft/sqlserver/jdbc/) — the main review focus
  • Tests (src/test/java/) — verify coverage
  • Build config (pom.xml, build.gradle) — dependency or plugin changes
  • Documentation (README.md, CHANGELOG.md, doc/) — doc updates
  • Resources (src/main/resources/, SQLServerResource.java) — error messages or config

2. Architectural Compliance

Verify the change follows project architecture rules (see .github/instructions/architecture.instructions.md):

• Source code follows the package structure under com.microsoft.sqlserver.jdbc
• TDS protocol changes are in IOBuffer.java, tdsparser.java, or Stream*.java files
• Connection property changes update SQLServerConnection.java, SQLServerDataSource.java, and SQLServerDriver.java
• New error messages are added to SQLServerResource.java using the R_ prefix convention
• The change compiles for all JRE profiles: jre8, jre11, jre17, jre21, jre25, jre26
• No platform-specific assumptions — driver must work on Windows, Linux, and macOS

3. Code Quality Review

Check against Coding_Guidelines.md and coding-best-practices.md:

Style

• Eclipse formatter mssql-jdbc_formatter.xml applied to changed code
• Javadoc comments on public methods (not required for simple getters/setters)
• null == variable pattern for null checks (project convention)
• camelCase for local variables and methods, PascalCase for class names
• Constants are UPPER_SNAKE_CASE

Best Practices

• Try-with-resources for Connection, Statement, ResultSet, streams, and sockets
• No unnecessary object creation in hot paths (tight loops, TDS parsing)
• Specific exception types caught (SQLTimeoutException, IOException) — not bare Exception
• Exceptions wrapped via SQLServerException.makeFromDriverError(...) with localized error keys
• Logger level checks before constructing log messages: if (logger.isLoggable(Level.FINER))
• No sensitive data (passwords, tokens, connection strings) in log messages or stack traces
• Shared mutable state is properly synchronized or volatile

4. Security Review

• No credentials, passwords, or secrets in code or comments
• No sensitive data logged via java.util.logging or exposed in exceptions
• SQL injection prevention via parameterized queries, even in metadata calls
• TLS/certificate settings handled safely (no blind trustServerCertificate=true in production code)
• Inputs validated to prevent injection in SQL or shell commands
• No unbounded data streams sent to server prior to authentication (e.g., in feature extensions)
• Sensitive char[] arrays (passwords, tokens) cleared after use; use SecureStringUtil for encrypted storage
• Always Encrypted key material handled via SQLServerColumnEncryptionKeyStoreProvider — not inline

5. Test Coverage Review

• Bug fixes have a test that would have caught the bug
• New features have unit tests and integration tests as appropriate
• Tests are in the correct package under src/test/java/com/microsoft/sqlserver/jdbc/
  • unit/ — verify whether the test is truly isolated or whether it still relies on AbstractTest connection setup and mssql_jdbc_test_connection_properties
  • Feature-specific packages (connection/, datatypes/, bulkCopy/, etc.) — typically integration tests
• Tests do NOT use hardcoded connection strings (use AbstractTest base class and test config when SQL Server access is required)
• Tests that require SQL Server are tagged with appropriate JUnit groups
• Tests excluded from default runs use correct group annotations (xSQLv12, xSQLv15, reqExternalSetup, fedAuth, kerberos, etc.)
• No flaky patterns (timing-dependent, order-dependent, resource-dependent)
• Consider state machine tests for complex state interactions (see docs/state-machine-testing.md)

6. API Design Review (if public API changed)

• Follows JDBC specification (4.2/4.3) for standard interfaces
• No breaking changes to existing public APIs
• New public methods have Javadoc comments
• New connection string properties are documented and have defaults preserving backward compatibility
• New ISQLServer* interfaces follow existing naming patterns
• Deprecated APIs have @Deprecated annotation with @since and migration guidance

7. Backward Compatibility

• Existing behavior is preserved for current consumers
• Default values for new connection properties maintain old behavior
• Driver works with SQL Server 2012+ and Azure SQL Database
• Changes compile across all JRE profiles (jre8 through jre26)
• If behavior changes, it's documented and opt-in (e.g., via connection property or feature flag)
• No JAR or package changes that break existing references

8. Performance Impact

• No new allocations on hot paths (TDS parsing, result set iteration)
• Buffers reused via preallocated arrays where possible
• No Java Stream API operations (.stream().filter().map()) on hot paths — use manual loops
• Caches or pools are bounded (not growing unbounded)
• Lazy initialization for expensive objects used conditionally
• For major features or large PRs, internal performance benchmarks should be run

9. Documentation and Changelog

• Javadoc comments on new/changed public APIs
• PR description references the issue (Fixes #...)
• PR description includes a self-contained summary (see review-process.md)
• CHANGELOG.md updated if user-visible behavior changes

10. Summary

Provide a review summary with:

• Verdict: Approve / Request Changes / Comment
• Strengths: What the PR does well
• Issues Found: Categorized as Critical / Major / Minor / Nit
• Suggestions: Optional improvements or alternative approaches
• Questions: Any clarifications needed from the author