Skip to content

Add ADR for DI#14466

Merged
calixtus merged 15 commits intoJabRef:mainfrom
InAnYan:feat/di-adr
Apr 17, 2026
Merged

Add ADR for DI#14466
calixtus merged 15 commits intoJabRef:mainfrom
InAnYan:feat/di-adr

Conversation

@InAnYan
Copy link
Copy Markdown
Member

@InAnYan InAnYan commented Nov 30, 2025

User description

Closes https://github.com/JabRef/jabref-issue-melting-pot/issues/590

Just added an ADR.

Steps to test

No steps to test. Just check the contents.

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • I manually tested my changes in running JabRef (always required)
  • [/] I added JUnit tests for changes (if applicable)
  • [/] I added screenshots in the PR description (if change is visible to the user)
  • [/] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
  • I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

PR Type

Documentation


Description

  • Adds ADR 0055 documenting dependency injection strategy

  • Defines hybrid approach: DI framework for JavaFX views, constructor-based DI for core logic

  • Explains rationale balancing JavaFX constraints with architectural clarity

  • Documents decision drivers, options considered, and verification methods


Diagram Walkthrough

flowchart LR
  A["DI Strategy Decision"] --> B["JavaFX Views<br/>DI Framework"]
  A --> C["Core Logic<br/>Constructor-based DI"]
  B --> D["Empty constructors<br/>Field injection"]
  C --> E["Explicit dependencies<br/>Testable"]
Loading

File Walkthrough

Relevant files
Documentation
0055-dependency-injection-approach.md
ADR for hybrid dependency injection approach                         

docs/decisions/0055-dependency-injection-approach.md

  • New ADR document establishing hybrid dependency injection strategy
  • Defines use of DI framework for JavaFX views with empty constructors
  • Specifies constructor-based DI for core logic layer
  • Documents decision drivers, considered options, consequences, and
    verification methods
+67/-0   

@InAnYan InAnYan marked this pull request as draft November 30, 2025 14:06
@InAnYan
Copy link
Copy Markdown
Member Author

InAnYan commented Nov 30, 2025

  • TODO: We use @Inject only in Views, not in ViewModels. And this again helps in testing, as ViewModel uses constructor-based DI

@koppor
Copy link
Copy Markdown
Member

koppor commented Dec 1, 2025

  • TODO @Inject for JAX-RS services - there, we use jakarta.inject.Inject and hk2: org.glassfish.hk2.api.ServiceLocator

@calixtus
Copy link
Copy Markdown
Member

@calixtus calixtus changed the title feat(docs): add ADR for DI Add ADR for DI Dec 12, 2025
@calixtus calixtus added dev: code-quality Issues related to code or architecture decisions dev: adr labels Dec 12, 2025
@InAnYan InAnYan marked this pull request as ready for review December 29, 2025 13:34
@InAnYan InAnYan added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 29, 2025
@qodo-free-for-open-source-projects

This comment was marked as resolved.

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects bot commented Dec 29, 2025

PR Code Suggestions ✨

Latest suggestions up to eab117d

CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Add a clear boundary rule

Add a concise "boundary rule" to the decision outcome to clearly define when to
use framework-based injection versus constructor-based DI.

docs/decisions/0055-dependency-injection-approach.md [28]

 Chosen option: "Use a mix of a DI framework and constructor-based DI", because this approach works with the constraints of JavaFX while preserving explicitness and testability in the rest of the system.
 
+Boundary rule: Framework-based injection is limited to framework-managed entry points (e.g., FXML-loaded controllers); core logic must remain framework-agnostic and use constructor-based DI.
+
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion adds a high-level, actionable "boundary rule" directly in the decision outcome, which is the most critical part of an ADR. This makes the key takeaway immediately clear and memorable for all developers.

Medium
Make confirmation rules unambiguous

Rewrite the confirmation rules as a clear checklist using MUST/SHOULD keywords
to make them unambiguous and easier to verify during code reviews.

docs/decisions/0055-dependency-injection-approach.md [39-41]

-Compliance can be verified by code review:  
-JavaFX views should not expose required constructor parameters, classes that are necessary for the view model are injected as fields in view class.
-Core logic classes should expose dependencies through constructors, with no field injection or framework-specific annotations.
+Compliance can be verified by code review:
 
+* JavaFX views/controllers MUST NOT require constructor parameters (to remain FXML-compatible).
+* Dependencies needed by the view model SHOULD be provided via framework-managed field/setter injection in the view/controller.
+* Core logic classes MUST expose dependencies via constructors (no field injection, no framework-specific annotations).
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This suggestion significantly improves the clarity and enforceability of the ADR's confirmation rules by structuring them as a checklist with standard RFC 2119 keywords, making them easier for developers to follow during code reviews.

Low
Scope framework rules precisely

Add more specific context to the framework usage rules, such as specifying
FXML-loaded for JavaFX controllers and scoping JAX-RS to the current runtime, to
make them more precise.

docs/decisions/0055-dependency-injection-approach.md [62-64]

-* JavaFX controllers: use `afterburnerfx` framework.
-* JAX-RS: use `jakarta.inject.Inject` and HK2 `org.glassfish.hk2.api.ServiceLocator`.
-* Other code: use constructor-based DI.
+* JavaFX controllers (FXML-loaded): use the `afterburnerfx` framework.
+* JAX-RS resources in our current runtime: use `jakarta.inject.Inject` with HK2 `org.glassfish.hk2.api.ServiceLocator`.
+* Other code (especially core logic): use constructor-based DI.
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion improves the precision of the ADR by clarifying the specific contexts for using each DI approach, which helps prevent future misinterpretations and incorrect implementations.

Low
  • Update

Previous suggestions

✅ Suggestions up to commit a966db5
CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Fix grammatical error in documentation
Suggestion Impact:The documentation sentence was corrected to use "too many" when referring to countable nouns ("arguments").

code diff:

-JavaFX imposes certain constraints on controllers and view models, especially regarding how FXML loads classes. At the same time, the rest of the app benefits from clear, explicit, and testable construction patterns. However, sometimes there are too much arguments in a constructor. The question is how to balance JavaFX requirements with the architectural clarity of constructor-based DI.
+JavaFX imposes certain constraints on controllers and view models, especially regarding how FXML loads classes. At the same time, the rest of the app benefits from clear, explicit, and testable construction patterns. However, sometimes there are too many arguments in a constructor. The question is how to balance JavaFX requirements with the architectural clarity of constructor-based DI.

Fix grammatical error: "too much" should be "too many" when referring to
countable nouns like arguments.

docs/decisions/0055-dependency-injection-approach.md [11]

-However, sometimes there are too much arguments in a constructor.
+However, sometimes there are too many arguments in a constructor.
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Correct grammatical errors in documentation to maintain professionalism and clarity

Low
Fix grammatical error in documentation
Suggestion Impact:The documentation sentence was corrected to use "too many" when referring to countable nouns ("arguments").

code diff:

-JavaFX imposes certain constraints on controllers and view models, especially regarding how FXML loads classes. At the same time, the rest of the app benefits from clear, explicit, and testable construction patterns. However, sometimes there are too much arguments in a constructor. The question is how to balance JavaFX requirements with the architectural clarity of constructor-based DI.
+JavaFX imposes certain constraints on controllers and view models, especially regarding how FXML loads classes. At the same time, the rest of the app benefits from clear, explicit, and testable construction patterns. However, sometimes there are too many arguments in a constructor. The question is how to balance JavaFX requirements with the architectural clarity of constructor-based DI.

Fix grammatical error: "as a fields" should be "as fields" (remove the article
"a" before the plural noun).

docs/decisions/0055-dependency-injection-approach.md [40]

-classes that are necessary for the view model are injected as a fields in view class.
+classes that are necessary for the view model are injected as fields in view class.
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Correct grammatical errors in documentation to maintain professionalism and clarity

Low

@github-actions github-actions bot added status: changes-required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Dec 29, 2025
@calixtus
Copy link
Copy Markdown
Member

Something is still thinking about is: To some extent our current solution for DI resembles in my eyes a ServiceLocator (see https://www.baeldung.com/java-service-locator-pattern#locator_di ). Is this true? Is this wanted? Don't we have to move to another solution? Do we have to include that in this PR?

@subhramit subhramit marked this pull request as draft January 27, 2026 20:35
InAnYan added 3 commits March 29, 2026 19:50
Clarified the approach to dependency injection by specifying the use of a DI framework for JavaFX view models and constructor-based DI for core code. Added analysis of the choice highlighting pros and cons.
@InAnYan InAnYan marked this pull request as ready for review March 29, 2026 17:59
@qodo-free-for-open-source-projects

This comment was marked as resolved.

@qodo-free-for-open-source-projects

This comment was marked as resolved.

Comment thread docs/decisions/0055-dependency-injection-approach.md Outdated
Comment thread docs/decisions/0055-dependency-injection-approach.md
@testlens-app

This comment has been minimized.

Added a header to the dependency injection decision record.
@testlens-app

This comment has been minimized.

@testlens-app

This comment has been minimized.

koppor
koppor previously requested changes Mar 29, 2026
Comment thread docs/decisions/0055-dependency-injection-approach.md Outdated
Corrected grammatical errors in the document.
@testlens-app

This comment has been minimized.

@testlens-app

This comment has been minimized.

@github-actions github-actions bot added status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Mar 30, 2026
@calixtus
Copy link
Copy Markdown
Member

What about my comment here, can you comment on this? #14466 (comment)

Added note on the similarity of afterburnerfx to the ServiceLocator pattern.
@testlens-app

This comment has been minimized.

Removed mention of 'afterburnerfx' and its similarity to the 'ServiceLocator' pattern.
@testlens-app
Copy link
Copy Markdown

testlens-app bot commented Mar 30, 2026

✅ All tests passed ✅

🏷️ Commit: 49484d5
▶️ Tests: 10210 executed
⚪️ Checks: 52/52 completed


Learn more about TestLens at testlens.app.

@InAnYan
Copy link
Copy Markdown
Member Author

InAnYan commented Mar 30, 2026

What about my comment here, can you comment on this? #14466 (comment)

Needs more thought from my side:

  • Study DI concept
  • Study ServiceLocator concept
  • Find how exactly afterburnerfx and DI is impemented in JabRef (you can't just call new on a class that uses @Inject
  • Then write a comment in "More information" section

@InAnYan
Copy link
Copy Markdown
Member Author

InAnYan commented Apr 12, 2026

I think the pr is good to go

The only problem is the koppor's comment

@calixtus
Copy link
Copy Markdown
Member

I think your rewording sounds fine. Integrate it. Then merge

@jabref-machine
Copy link
Copy Markdown
Collaborator

JUnit tests of jabsrv are failing. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Source Code Tests / Unit tests (pull_request)" and click on it.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

@github-actions github-actions bot added status: changes-required Pull requests that are not yet complete and removed status: no-bot-comments labels Apr 12, 2026
@InAnYan
Copy link
Copy Markdown
Member Author

InAnYan commented Apr 13, 2026

Done

@calixtus
Copy link
Copy Markdown
Member

Lets move on here. If something comes up or we learn something new, this can always be revised and updated.

@calixtus calixtus merged commit 54bccf1 into JabRef:main Apr 17, 2026
51 of 55 checks passed
@calixtus
Copy link
Copy Markdown
Member

Merging doc without queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev: adr dev: code-quality Issues related to code or architecture decisions 📌 Pinned Review effort 1/5 status: changes-required Pull requests that are not yet complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants