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

feat: Add interfaces for CRUD services #20743

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

feat: Add interfaces for CRUD services #20743

wants to merge 18 commits into from

Conversation

Artur-
Copy link
Member

@Artur- Artur- commented Dec 18, 2024

These were previously in Hilla but are equally useful in Flow applications

Copy link

github-actions bot commented Dec 18, 2024

Test Results

1 162 files  + 3  1 162 suites  +3   1h 30m 41s ⏱️ - 5m 10s
7 603 tests +35  7 547 ✅ +35  56 💤 ±0  0 ❌ ±0 
7 944 runs  +12  7 881 ✅ +15  63 💤  - 3  0 ❌ ±0 

Results for commit 8a9f6b6. ± Comparison against base commit df40b75.

♻️ This comment has been updated with latest results.

These were previously in Hilla but are equally useful in Flow applications
@Artur- Artur- requested a review from peholmst December 18, 2024 16:17
Copy link

sonarcloud bot commented Dec 18, 2024

Copy link
Member

@peholmst peholmst left a comment

Choose a reason for hiding this comment

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

Since this is a breaking change anyway, I took the liberty of being extra picky with the review.

* @return the fresh object or {@code null} if no object was found to update
*/
@Nullable
T save(T value);
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for returning null instead of using an Optional or throwing an exception?

@@ -0,0 +1,2 @@
@org.springframework.lang.NonNullApi
package com.vaadin.flow.spring.data.jpa;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add @NullMarked from JSpecify?

@@ -0,0 +1,2 @@
@org.springframework.lang.NonNullApi
package com.vaadin.flow.spring.data;
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here?

* the filter to apply or {@code null} to not filter
* @return a list of objects or an empty list if no objects were found
*/
@NonNull
Copy link
Member

Choose a reason for hiding this comment

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

Are the @NonNull annotations here because of Hilla's code generator? I guess Hilla does not understand @NonNullApi / @NullMarked on a parent interface?

@JsonSubTypes({ @Type(value = OrFilter.class, name = "or"),
@Type(value = AndFilter.class, name = "and"),
@Type(value = PropertyStringFilter.class, name = "propertyString") })
public class Filter implements Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

Are the Filter classes meant to mainly be created by Jackson, or is there a situation where a developer may want to create them in Java as well? Also why are the Filter classes mutable and not immutable?

@@ -0,0 +1,2 @@
@org.springframework.lang.NonNullApi
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding @NullMarked here.

*/
public List<T> saveAll(Iterable<T> values) {
List<T> saved = new ArrayList<>();
getRepository().saveAll(values).forEach(saved::add);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use List.copyOf(..) instead here?

import com.vaadin.flow.spring.data.filter.PropertyStringFilter;
import org.springframework.data.jpa.domain.Specification;

public class PropertyStringFilterSpecification<T> implements Specification<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be package private, or is there any situation where you would want to manually instantiate it or extend it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants