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

DATAJPA-1303. Added support ignoring case for finding collections #261

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@krupt
Copy link
Contributor

commented Mar 24, 2018

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).
@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Mar 24, 2018

@krupt Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Mar 24, 2018

@krupt Thank you for signing the Contributor License Agreement!

private final Type type;
private final ParameterExpression<T> expression;
private final PersistenceProvider persistenceProvider;

/**
* Creates a new {@link ParameterMetadata}.
*/
public ParameterMetadata(ParameterExpression<T> expression, Type type, @Nullable Object value,

This comment has been minimized.

Copy link
@odrotbohm

odrotbohm Mar 26, 2018

Member

Can you clarify why this change is needed? I don't feel comfortable with making ParameterMetadata aware of anything ignore-case related. Shouldn't the handling of that be applied in the code eventually calling ….prepare(…)?

This comment has been minimized.

Copy link
@krupt

krupt Mar 26, 2018

Author Contributor

I need more time to study this proposal

This comment has been minimized.

Copy link
@krupt

krupt Mar 26, 2018

Author Contributor

I can't find better solution. Method .prepare calling only from org.springframework.data.jpa.repository.query.QueryParameterSetterFactory.CriteriaQueryParameterSetterFactory#CriteriaQueryParameterSetterFactory, but there have not place for this. In ParameterMetadata we prepare another string conversion and can do this conversion. I think that I should change ignoreCaseType field to boolean

@@ -232,19 +237,19 @@ public Object prepare(Object value) {

switch (type) {
case STARTING_WITH:
return String.format("%s%%", value.toString());

This comment has been minimized.

Copy link
@odrotbohm

odrotbohm Mar 26, 2018

Member

Please avoid changes unrelated to the actual improvement we're trying to fix.

This comment has been minimized.

Copy link
@krupt

krupt Mar 26, 2018

Author Contributor

Should I create another issue in JIRA to fix creation of unnecessary objects?

This comment has been minimized.

Copy link
@odrotbohm

odrotbohm Mar 26, 2018

Member

Unless you have detailed profiling numbers showing that that change has significant impact on the performance I don't think it's worth the effort.

default:
return value;
}
}

return Collection.class.isAssignableFrom(expressionType) //

This comment has been minimized.

Copy link
@odrotbohm

odrotbohm Mar 26, 2018

Member

Please avoid changes unrelated to the actual improvement we're trying to fix.

@@ -1175,6 +1175,48 @@ public void findByElementCollectionAttribute() {
assertThat(result).containsOnly(firstUser, secondUser);
}

@Test

This comment has been minimized.

Copy link
@odrotbohm

odrotbohm Mar 26, 2018

Member

Please add references to the JIRA issue like the other test methods do. Also, please add new test methods last, not in the middle of others.

@@ -290,6 +290,12 @@
// DATAJPA-496
List<User> findByAttributesIn(Set<String> attributes);

List<User> findByAttributesIgnoreCaseIn(Collection<String> attributes);

This comment has been minimized.

Copy link
@odrotbohm

odrotbohm Mar 26, 2018

Member

Please move those methods to the bottom of the file and mark them with the JIRA ticket because of which they were introduced.

@krupt krupt force-pushed the krupt:master branch from 719a85a to 3e0c01e Apr 23, 2018

@schauder schauder force-pushed the spring-projects:master branch from e164784 to 01e36db Nov 1, 2018

schauder added a commit that referenced this pull request Jul 8, 2019

schauder added a commit that referenced this pull request Jul 8, 2019

DATAJPA-1303 - Polishing.
Code formatting.

Original pull request: #261.
@schauder

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Thanks, that is squashed, polished and merged.

@schauder schauder closed this Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.