Coding best practices and review process for mssql-jdbc

.github/PULL_REQUEST_TEMPLATE/pr-template.md

@@ -0,0 +1,35 @@
+## Description
+
+Provide a summary of the changes being introduced. Important topics to cover
+include:
+
+- Description of the functionality.
+- API changes, backwards compatibility, deprecations, etc.
+- Documentation, localization.
+- Bug fixes.
+- Code hygiene, refactoring, improvements.
+- Engineering processes (CI, pipelines, test coverage)
+
+High quality descriptions will lead to a smoother review experience.
+
+## Issues
+
+Link to any relevant issues, bugs, or discussions (e.g., "Closes \#123", "Fixes
+issue \#456").
+
+## Testing
+
+Describe the automated tests (unit, integration) you created or modified.
+Provide justification for any gap in automated testing.  
+List any manual testing steps that were performed to ensure the changes work.  
+Mention a link to successful test runs in ADO
+
+## Guidelines
+
+Please review the contribution guidelines before submitting a pull request:
+
+- [Contributing](/CONTRIBUTING.md)
+- [Code of Conduct](/CODE_OF_CONDUCT.md)
+- [Best Practices](/coding-best-practices.md)
+- [Coding Guidelines](/coding-guidelines.md)
+- [Review Process](/review-process.md)

CODE_OF_CONDUCT.md

@@ -0,0 +1,3 @@
+# Code of Conduct
+
+This project has adopted the [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/). For more information see the [Code of Conduct FAQ](https://opensource.microsoft.com/codeofconduct/faq/) or contact [[email protected]](mailto:[email protected]) with any additional questions or comments.

CONTRIBUTING.md

@@ -0,0 +1,9 @@
+- **DO** report each issue as a new issue (but check first if it's already been reported)
+- **DO** respect Issue Templates and provide detailed information. It will make the process to reproduce the issue and provide a fix faster.
+- **DO** provide a minimal repro app demonstrating the problem in isolation will greatly speed up the process of identifying and fixing problems.
+- **DO** follow our [coding guidelines](coding-guidelines.md) when working on a Pull Request.
+- **DO** follow our [coding best practices](coding-best-practices.md) when working on a Pull Request.
+- **DO** follow our [review process](review-process.md) when reviwing a Pull Request.
+- **DO** give priority to the current style of the project or file you're changing even if it diverges from the general guidelines.
+- **DO** consider cross-platform compatibility and supportability for all supported SQL and Azure Servers and client configurations.
+- **DO** include tests when adding new features. When fixing bugs, start with adding a test that highlights how the current behavior is broken.

README.md

@@ -184,6 +184,17 @@ When setting 'useFmtOnly' property to 'true' for establishing a connection or cr
 </dependency>
 ```
 
+## Helpful Links
+
+| Topic | Link to File |
+| :---- | :------------- |
+| Coding Guidelines | [coding-guidelines.md](/coding-guidelines.md) |
+| Best Practices | [coding-best-practices.md](/coding-best-practices.md) |
+| Review Process | [review-process.md](/review-process.md) |
+| Security Guidelines | [security.md](/security.md) |
+| Changelog | [CHANGELOG.md](CHANGELOG.md) |
+| Code of Conduct | [CODE_OF_CONDUCT.md](CODE_OF_CONDUCT.md) |
+
 ## Guidelines for Creating Pull Requests
 We love contributions from the community.  To help improve the quality of our code, we encourage you to use the mssql-jdbc_formatter.xml formatter provided on all pull requests.
 

coding-best-practices.md

@@ -0,0 +1,94 @@
+# Coding Best Practices
+
+This document describes some typical programming pitfalls and best practices
+related to Java and JDBC. It will grow and change as we encounter new situations
+and the codebase evolves.
+
+## Correctness & Business Logic
+
+- Validate if the code adheres to what it is supposed to do. Compare the
+  implementation with the specification and ask questions, to get clarity.
+
+## Memory / Resource Management
+
+- Use try-with-resources for managing ResultSet, Statement, Connection, streams, sockets, file handles.  
+- Avoid unnecessary object creation in hot paths (tight loops, parsing routines). 
+- Reuse preallocated buffers if available (e.g., for TDS packets).
+- Can memory usage be optimized? Is pooling or reuse possible?
+
+## Error Handling & Reliability  
+
+- Catch specific exception types (SQLTimeoutException, IOException, etc.).  
+- Wrap exceptions using SQLServerException.makeFromDriverError(...) or similar APIs. 
+- Ensure logs are useful and include information such as connection ID or any state transitions. 
+- Avoid catching the base Exception unless absolutely required.
+
+## Async, Concurrency & Thread Safety (if applicable)
+
+- Synchronize access to shared mutable state.
+- For background threads, handle interruption and termination gracefully.
+- Avoid deadlocks by keeping lock hierarchy consistent.
+- Are all shared variables properly synchronized or volatile ?
+- Are ExecutorServices or thread pools properly shut down?
+
+## Backward Compatibility
+
+- Verify unit tests and integration tests haven’t regressed (especially server compatibility tests).
+- Preserve public interfaces and behaviors unless part of a breaking release plan.
+- Annotate deprecated methods if replacing functionality.
+- Are any existing APIs modified or removed? If yes, is this justified and documented?
+- Could this change affect driver users on older SQL Server versions or JDBC clients?
+- Are test expectations changed in a way that signals a behavior shift?
+
+## Security Considerations
+
+- Never log passwords, secrets, or connection strings with credentials.
+- Validate inputs to avoid SQL injection, even on metadata calls.
+- Are there any user inputs going into SQL or shell commands directly?
+- Are secrets being logged or exposed in stack traces? 
+- Are TLS/certificate settings handled safely and explicitly?
+- Are we sending unbounded data streams to server prior to authentication e.g. in feature extensions?
+
+## Performance & Scalability  
+
+- Avoid blocking operations on performance-critical paths.
+- Profile memory allocations or TDS I/O if large buffers are introduced.
+- Use lazy loading for large metadata or result sets.
+- Will this impact startup, connection, or execution latency? 
+- Could this increase memory usage, thread contention, or GC pressure?
+- Are any caches or pools growing unbounded?
+- For major features or large PRs, always run the internal performance benchmarks or performance 
+  testsuite to determine if the new changes are causing any performance degradation.
+
+## Observability (Logging / Tracing / Metrics)  
+
+- Use existing logging framework (java.util.logging) for debug/trace with appropriate logging level.
+- Include connection/session ID when logging.
+- Ensure log messages are actionable and contextual.
+- Is the new code adequately traceable via logs?
+- Are any logs too verbose or leaking user data?
+
+## Unit Tests / Integration
+
+- Have you added unit tests and integration tests?
+- Unit tests should not depend on external resources.  Code under unit test
+  should permit mocked resources to be provided/injected.
+- Avoid tests that rely on timing.
+
+
+## Configuration & Feature Flags  
+
+- Is the change considered a breaking change? If breaking change is not
+  avoidable, has a Configuration/Feature flag been introduced for customers to
+  revert to old behavior?
+
+## Code Coverage expectations
+
+- Does the new code have sufficient test coverage? 
+- Are the core paths exercised (including error conditions and retries)? 
+- If the code is untestable, is it marked as such and is there a reason (e.g., hard dependency on native SQL Server behavior)?
+
+## Pipeline runs
+
+- Is the CI passing? If not, then have you looked at the failures, and found the
+  cause for failures?

review-process.md

@@ -0,0 +1,74 @@
+# Review Process
+
+This document describes guidelines and responsibilities for participants of
+pull request reviews.
+
+## Author Responsibilities
+
+- **Describe the problem up front.**  In the PR description, include a concise,
+  self-contained summary of the issue being solved and link to relevant specs,
+  issues, or discussions.  For open-source PRs, capture the context in one clear
+  section so reviewers don’t have to sift through comment threads.
+- **Ship code with tests.**  Submit unit and integration tests alongside the
+  implementation to demonstrate expected behavior and prevent regressions.
+- **Instrument for production.**  Add meaningful tracing or logging so the
+  changes can be monitored and debugged once deployed.
+- **Keep PRs focused.**  Split large or mixed-purpose changes into smaller,
+  logical units (e.g., separate refactors from feature work).
+- **Highlight important changes.**  Add your own comments to the diffs to call
+  out important changes that require extra scrutiny, or to explain a change
+  whose inclusion isn't self-evident.
+- **Choose your reviewers.**  The author should assign an initial set of
+  required and optional reviewers.  Reviewers may change their role later after
+  discussing with the author.
+- **Any open issue blocks completion.**  Do not complete a review with open
+  issues.  All issues must be resolved by the reviewer who created them, or
+  another reviewer delegate.  Do not override tooling that prevents completion.
+- **Do not resolve reviewer issues.**  Only reviewers should resolve reviewer
+  issues.  Ideally the creator of the issue should resolve it, but reviewers may
+  close each other's issues under certain circumstances.
+
+## Reviewer Responsibilities
+
+- **Understand what you’re approving.**  If anything is unclear, ask questions.
+  Require in-code comments that explain why a solution exists - not just what it
+  does.  PR comments are not a substitute for code comments.
+- **Read the full PR description.**  Context matters; don’t skip it.
+- **Review the changes, not the design.**  A code review is not the time to
+  review a design choice.  If you think an alternative design would be better,
+  discuss with the author and team, and ask for a new PR if the new design is
+  implemented instead.
+- **Demand tests.**  If appropriate tests are missing, request them.  When
+  testing truly isn’t feasible, insist on a documented rationale.
+- **Note partial reviews.**  If you reviewed only part of the code, say so in
+  the PR.
+- **Delegate wisely.**  If you cannot complete your reviewer role's
+  responsibilities, delegate to another subject-matter expert.
+- **Own your approval.**  You are accountable for the quality and correctness of
+  the code you sign off on.
+- **Block completion on all open issues.**  Any issue you open against a review
+  must be resolved to your satisfaction before a review may complete.
+  Resolution may include new code changes to fix a problem, new in-code
+  documentation to explain something, or a discussion within the PR itself.
+- **Do not close other reviewer's issues.** The reviewer who created an issue
+  should be the one to resolve it.  Exceptions include explicit delegation or
+  OOTO absences.  Any delegations should be discussed with the team.
+- **Never rubber-stamp.**  Trust the author but verify the code - always conduct
+  a real review.
+- **Review in a timely manner.**  Reviewers should aim to perform a review
+  within 2 business days of receiving the initial request, and 1 business day
+  for follow-up changes.  Reviewing a PR is higher priority than most other
+  tasks.
+
+## Backwards Compatibility Awareness
+
+Subtle changes may have backwards compatibility implications.  Below are some of
+the code changes and side-effects to watch out for:
+
+- Do these changes force updates to existing integration tests - a sign they may
+  introduce a breaking change?
+- Will these changes modify the public API’s behavior in a way that could break
+  existing applications when they upgrade to the new binaries?
+- For any non-security, breaking changes that impact customers, have we provided
+  an opt-out - such as a connection-string flag, AppContext switch, or similar
+  setting - that lets users revert to the previous behavior?