Skip to content

Commit f393ca7

Browse files
machavanmachavan
authored
Coding best practices and review process for mssql-jdbc (#2666)
* Coding best practices and review process for mssq-jdbc * Minor edits to helpful links * Edited link to coding guidelines
1 parent 8162706 commit f393ca7

File tree

6 files changed

+226
-0
lines changed

6 files changed

+226
-0
lines changed

‎.github/PULL_REQUEST_TEMPLATE/pr-template.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
## Description
2+
3+
Provide a summary of the changes being introduced. Important topics to cover
4+
include:
5+
6+
- Description of the functionality.
7+
- API changes, backwards compatibility, deprecations, etc.
8+
- Documentation, localization.
9+
- Bug fixes.
10+
- Code hygiene, refactoring, improvements.
11+
- Engineering processes (CI, pipelines, test coverage)
12+
13+
High quality descriptions will lead to a smoother review experience.
14+
15+
## Issues
16+
17+
Link to any relevant issues, bugs, or discussions (e.g., "Closes \#123", "Fixes
18+
issue \#456").
19+
20+
## Testing
21+
22+
Describe the automated tests (unit, integration) you created or modified.
23+
Provide justification for any gap in automated testing.
24+
List any manual testing steps that were performed to ensure the changes work.
25+
Mention a link to successful test runs in ADO
26+
27+
## Guidelines
28+
29+
Please review the contribution guidelines before submitting a pull request:
30+
31+
- [Contributing](/CONTRIBUTING.md)
32+
- [Code of Conduct](/CODE_OF_CONDUCT.md)
33+
- [Best Practices](/coding-best-practices.md)
34+
- [Coding Guidelines](/coding-guidelines.md)
35+
- [Review Process](/review-process.md)

‎CODE_OF_CONDUCT.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# Code of Conduct
2+
3+
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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
- **DO** report each issue as a new issue (but check first if it's already been reported)
2+
- **DO** respect Issue Templates and provide detailed information. It will make the process to reproduce the issue and provide a fix faster.
3+
- **DO** provide a minimal repro app demonstrating the problem in isolation will greatly speed up the process of identifying and fixing problems.
4+
- **DO** follow our [coding guidelines](coding-guidelines.md) when working on a Pull Request.
5+
- **DO** follow our [coding best practices](coding-best-practices.md) when working on a Pull Request.
6+
- **DO** follow our [review process](review-process.md) when reviwing a Pull Request.
7+
- **DO** give priority to the current style of the project or file you're changing even if it diverges from the general guidelines.
8+
- **DO** consider cross-platform compatibility and supportability for all supported SQL and Azure Servers and client configurations.
9+
- **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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,17 @@ When setting 'useFmtOnly' property to 'true' for establishing a connection or cr
184184
</dependency>
185185
```
186186

187+
## Helpful Links
188+
189+
| Topic | Link to File |
190+
| :---- | :------------- |
191+
| Coding Guidelines | [coding-guidelines.md](/coding-guidelines.md) |
192+
| Best Practices | [coding-best-practices.md](/coding-best-practices.md) |
193+
| Review Process | [review-process.md](/review-process.md) |
194+
| Security Guidelines | [security.md](/security.md) |
195+
| Changelog | [CHANGELOG.md](CHANGELOG.md) |
196+
| Code of Conduct | [CODE_OF_CONDUCT.md](CODE_OF_CONDUCT.md) |
197+
187198
## Guidelines for Creating Pull Requests
188199
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.
189200

‎coding-best-practices.md

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
# Coding Best Practices
2+
3+
This document describes some typical programming pitfalls and best practices
4+
related to Java and JDBC. It will grow and change as we encounter new situations
5+
and the codebase evolves.
6+
7+
## Correctness & Business Logic
8+
9+
- Validate if the code adheres to what it is supposed to do. Compare the
10+
implementation with the specification and ask questions, to get clarity.
11+
12+
## Memory / Resource Management
13+
14+
- Use try-with-resources for managing ResultSet, Statement, Connection, streams, sockets, file handles. 
15+
- Avoid unnecessary object creation in hot paths (tight loops, parsing routines).
16+
- Reuse preallocated buffers if available (e.g., for TDS packets).
17+
- Can memory usage be optimized? Is pooling or reuse possible?
18+
19+
## Error Handling & Reliability 
20+
21+
- Catch specific exception types (SQLTimeoutException, IOException, etc.). 
22+
- Wrap exceptions using SQLServerException.makeFromDriverError(...) or similar APIs.
23+
- Ensure logs are useful and include information such as connection ID or any state transitions.
24+
- Avoid catching the base Exception unless absolutely required.
25+
26+
## Async, Concurrency & Thread Safety (if applicable)
27+
28+
- Synchronize access to shared mutable state.
29+
- For background threads, handle interruption and termination gracefully.
30+
- Avoid deadlocks by keeping lock hierarchy consistent.
31+
- Are all shared variables properly synchronized or volatile ?
32+
- Are ExecutorServices or thread pools properly shut down?
33+
34+
## Backward Compatibility
35+
36+
- Verify unit tests and integration tests haven’t regressed (especially server compatibility tests).
37+
- Preserve public interfaces and behaviors unless part of a breaking release plan.
38+
- Annotate deprecated methods if replacing functionality.
39+
- Are any existing APIs modified or removed? If yes, is this justified and documented?
40+
- Could this change affect driver users on older SQL Server versions or JDBC clients?
41+
- Are test expectations changed in a way that signals a behavior shift?
42+
43+
## Security Considerations
44+
45+
- Never log passwords, secrets, or connection strings with credentials.
46+
- Validate inputs to avoid SQL injection, even on metadata calls.
47+
- Are there any user inputs going into SQL or shell commands directly?
48+
- Are secrets being logged or exposed in stack traces?
49+
- Are TLS/certificate settings handled safely and explicitly?
50+
- Are we sending unbounded data streams to server prior to authentication e.g. in feature extensions?
51+
52+
## Performance & Scalability 
53+
54+
- Avoid blocking operations on performance-critical paths.
55+
- Profile memory allocations or TDS I/O if large buffers are introduced.
56+
- Use lazy loading for large metadata or result sets.
57+
- Will this impact startup, connection, or execution latency?
58+
- Could this increase memory usage, thread contention, or GC pressure?
59+
- Are any caches or pools growing unbounded?
60+
- For major features or large PRs, always run the internal performance benchmarks or performance
61+
testsuite to determine if the new changes are causing any performance degradation.
62+
63+
## Observability (Logging / Tracing / Metrics) 
64+
65+
- Use existing logging framework (java.util.logging) for debug/trace with appropriate logging level.
66+
- Include connection/session ID when logging.
67+
- Ensure log messages are actionable and contextual.
68+
- Is the new code adequately traceable via logs?
69+
- Are any logs too verbose or leaking user data?
70+
71+
## Unit Tests / Integration
72+
73+
- Have you added unit tests and integration tests?
74+
- Unit tests should not depend on external resources.  Code under unit test
75+
should permit mocked resources to be provided/injected.
76+
- Avoid tests that rely on timing.
77+
78+
79+
## Configuration & Feature Flags 
80+
81+
- Is the change considered a breaking change? If breaking change is not
82+
avoidable, has a Configuration/Feature flag been introduced for customers to
83+
revert to old behavior?
84+
85+
## Code Coverage expectations
86+
87+
- Does the new code have sufficient test coverage?
88+
- Are the core paths exercised (including error conditions and retries)?
89+
- If the code is untestable, is it marked as such and is there a reason (e.g., hard dependency on native SQL Server behavior)?
90+
91+
## Pipeline runs
92+
93+
- Is the CI passing? If not, then have you looked at the failures, and found the
94+
cause for failures?

‎review-process.md

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

0 commit comments

Comments
 (0)