Skip to content

Conversation

@gnodet
Copy link
Contributor

@gnodet gnodet commented Jun 3, 2025

When injecting List dependencies, the DI container now sorts the
bindings by their @priority annotation value in descending order
(highest priority first) to ensure deterministic ordering.

This change ensures that components with higher priority values
appear first in injected lists, providing predictable behavior
for dependency injection scenarios where order matters.

  • Modified InjectorImpl.doGetCompiledBinding() to sort bindings
    by priority before creating supplier lists
  • Added comprehensive tests for priority-based list ordering
  • Includes tests for mixed priorities and default priority handling

JIRA issue: MNG-8764

When injecting List<T> dependencies, the DI container now sorts the
bindings by their @priority annotation value in descending order
(highest priority first) to ensure deterministic ordering.

This change ensures that components with higher priority values
appear first in injected lists, providing predictable behavior
for dependency injection scenarios where order matters.

- Modified InjectorImpl.doGetCompiledBinding() to sort bindings
  by priority before creating supplier lists
- Added comprehensive tests for priority-based list ordering
- Includes tests for mixed priorities and default priority handling
@gnodet gnodet requested a review from cstamas June 3, 2025 18:56
@gnodet gnodet added this to the 4.0.0-rc-4 milestone Jun 3, 2025
Copy link
Contributor

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done.

might consider IoC (Dependency Inversion Principle), as part of SOLID principles afterward.

List<Supplier<Object>> list =
sortedBindings.stream().map(this::compile).collect(Collectors.toList());
//noinspection unchecked
return () -> (Q) list(list, Supplier::get);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering this domain logic, might use OOP implementing comparable as its wont hurt nobody giving special attention and cohesion to core logic.

image

sortedBindings.sort(comparing.reversed());

List<Supplier<Object>> list =
sortedBindings.stream().map(this::compile).collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List<Binding<Object>> sortedBindings = new ArrayList<>(res2);
sortedBindings.sort(Comparator.reverseOrder());
//noinspection unchecked
return () -> (Q) list(sortedBindings.stream().map(this::compile).collect(Collectors.toList()), Supplier::get);
    @Override
    public int compareTo(Binding<Object> objectBinding) {
        return Integer.compare(this.priority, objectBinding.priority);
    }

@gnodet
Copy link
Contributor Author

gnodet commented Jun 3, 2025

well done.

might consider IoC (Dependency Inversion Principle), as part of SOLID principles afterward.

This is an IoC container, I wouldn't want to use IoC to bootstrap it ;-)
Please try to look and understand the code you're talking about...

@Pankraz76
Copy link
Contributor

wouldn't want to use IoC

why not? IoC is a pattern generally valid and possible to apply in this case as well.

Current implementation seems to be entangled with feature envy, as the sorting logic is an random impl. detail, not of interest to the actual InjectorImpl.java.

Its task it to apply to sorting not to actually do it.

Instead of delegating the topic to the real one, its done by its consumer, making the code impossible to reuse.

@gnodet
Copy link
Contributor Author

gnodet commented Jun 4, 2025

wouldn't want to use IoC

why not? IoC is a pattern generally valid and possible to apply in this case as well.

Did you read the full sentence ? Maven DI is an IoC container. It should not depend on another IoC container to actually be created. The other parts of Maven do rely on Maven DI, or Sisu containers for IoC. But specific bits, and especially the InjectorImpl class is one of the core implementation class of the IoC container.

@Pankraz76
Copy link
Contributor

Pankraz76 commented Jun 4, 2025

Its just about moving the external comparator (feature envy) to the actual domain (business logic, following OOP) making it comparable. No injection, no nothing. Just plain old java.

Imho this has nothing to do with an actual container, or what so ever. Im not injecting anything its just a concept of inverting the logic.

The topic i want to talk about does not know about container or whatever.

Its about giving SOC, avoiding feature envy leading to coupling instead of cohesion.

we can merge this and evolve design afterward, its no problem. I will try if my theory is working.

// Sort bindings by priority (highest first) for deterministic ordering
List<Binding<Object>> sortedBindings = new ArrayList<>(res2);
Comparator<Binding<Object>> comparing = Comparator.comparing(Binding::getPriority);
sortedBindings.sort(comparing.reversed());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sortedBindings.sort(comparing.reversed());
sortedBindings.sort(Binding.getPriorityComparator());

could shift feature envy in dedicated domain Binding.

public Comparator<Binding<Object>> getPriorityComparator() {
    return Comparator.comparing(Binding::getPriority);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might do this, as leaner than implementing comparable. Both better then this to avoid exact this situation:

image

DRY DRY DRY

@gnodet
Copy link
Contributor Author

gnodet commented Jun 4, 2025

Its just about moving the external comparator (feature envy) to the actual domain (business logic, following OOP) making it comparable. No injection, no nothing. Just plain old java.

Agreed, it's plain old java, but it's unrelated to IOC, which is generally about injecting dependencies in components rather than letting them create their own dependencies.

we can merge this and evolve design afterward, its no problem. I will try if my theory is working.

Yes, please raise a JIRA issue and a PR. The Bindings are sorted in a few places in the code, and IIRC, they are always sorted according to reversed priority. So having them implementing Comparable should definitely be a possible enhancement.

@Pankraz76
Copy link
Contributor

IOC

yes, i mean the concept behind it, not some random impl. detail.

@Pankraz76
Copy link
Contributor

So having them implementing Comparable should definitely be a possible enhancement.

merci.

@gnodet gnodet merged commit 6fac2e5 into apache:master Jun 4, 2025
19 checks passed
@gnodet gnodet added bug Something isn't working and removed maintenance labels Jun 4, 2025
Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request Jun 4, 2025
When injecting List<T> dependencies, the DI container now sorts the
bindings by their @priority annotation value in descending order
(highest priority first) to ensure deterministic ordering.

This change ensures that components with higher priority values
appear first in injected lists, providing predictable behavior
for dependency injection scenarios where order matters.

- Modified InjectorImpl.doGetCompiledBinding() to sort bindings
  by priority before creating supplier lists
- Added comprehensive tests for priority-based list ordering
- Includes tests for mixed priorities and default priority handling
@jira-importer
Copy link

Resolve #9344

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants