Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce MethodParameterFactory to Parameters #2996

Closed
wants to merge 2 commits into from
Closed

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Dec 4, 2023

We now provide a factory function to create MethodParameter objects associated with the enclosing class so parameters can resolve generics properly.

@mp911de mp911de added the type: bug A general bug label Dec 4, 2023
@mp911de mp911de added this to the 3.2.1 (2023.1.1) milestone Dec 4, 2023
@mp911de mp911de force-pushed the issue/2995 branch 2 times, most recently from a997fd5 to 42953ce Compare December 4, 2023 10:10
@mp911de mp911de linked an issue Dec 4, 2023 that may be closed by this pull request
*
* @param method must not be {@literal null}.
* @param aggregateType must not be {@literal null}.
*/
public DefaultParameters(Method method, TypeInformation<?> aggregateType) {
super(method, param -> new Parameter(param, aggregateType));
this(method, index -> new MethodParameter(method, index), aggregateType);
Copy link
Member

Choose a reason for hiding this comment

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

should we consider the aggregate type here and pass it on via withContainingClass?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the constructor signature, I didn't want to change existing methods. I have the feeling that introducing a constructor taking RepositoryMetadata would be helpful.

Copy link
Member

@odrotbohm odrotbohm left a comment

Choose a reason for hiding this comment

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

Can you elaborate why you think creating the dependency to the RepositoryMetadata is needed? It looks like it's solely used to pull out the domain type on construction? So why not stay with the variant that takes a TypeInformation and extract that on creation of the parameters?

public Iterator<T> iterator() {
return parameters.iterator();
}

Copy link
Member

Choose a reason for hiding this comment

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

Superfluos?

@mp911de
Copy link
Member Author

mp911de commented Dec 5, 2023

We have two dependencies (repository interface, domain type) now that we derive from RepositoryMetadata. Instead passing on each one individually with a growing constructor, it makes sense to pass on RepositoryMetadata. We already depend on the repository.core package so design-wise it seemed a good fit.

@odrotbohm
Copy link
Member

I might be missing something but neither Parameters nor DefaultParameters previously depended on some repository abstraction, did they? It's all pure Method and TypeInformation. So I wonder, why we would now force callers of these constructors to first create a RepositoryMetadata that needs a full repository interface, when the TypeInformation for the aggreagte type is just enough?

@mp911de
Copy link
Member Author

mp911de commented Dec 5, 2023

This change isn't about the domain type, it is about methodParameter.withContainingClass(repositoryMetadata.getRepositoryInterface());.

@odrotbohm
Copy link
Member

Are you sure, you're discussing the right PR? I see changes in the constructors for Parameters and DefaultParameters to eventually end up at metadata.getDomainTypeInformation(). 🤔

We now provide a ParametersSource object to create MethodParameter objects associated with the enclosing class so parameters can resolve generics properly.
@mp911de mp911de closed this in bd05ed1 Dec 8, 2023
mp911de added a commit that referenced this pull request Dec 8, 2023
mp911de added a commit that referenced this pull request Dec 8, 2023
We now provide a ParametersSource object to create MethodParameter objects associated with the enclosing class so parameters can resolve generics properly.

Closes #2996
@mp911de mp911de deleted the issue/2995 branch December 8, 2023 10:05
mp911de added a commit that referenced this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter.getType() returns erased generic type for method parameters
3 participants