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

use conditions instead of criteria as workaround #18294

Merged

Conversation

atomfrede
Copy link
Member

Instead of using criteria for where clause this workaround uses conditions directly where needed (find by id).

closes #18269


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@atomfrede atomfrede force-pushed the 18269-workaround-removal-of-criteria branch from 5a37a9f to c220a21 Compare April 5, 2022 20:48
deepu105
deepu105 previously approved these changes Apr 6, 2022
} else {
return createSelectImpl(selectFrom.limitOffset(pageable.getPageSize(), pageable.getOffset()), entityType, pageable.getSort());
}
return createSelectImpl(selectFrom.limitOffset(pageable.getPageSize(), pageable.getOffset()), entityType, pageable.getSort());
Copy link
Member

@deepu105 deepu105 Apr 6, 2022

Choose a reason for hiding this comment

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

should we pass the condition here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think you're right. Hope to be able to update in during lunch.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can merge once that is done

Copy link
Member

Choose a reason for hiding this comment

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

I can also update it

@deepu105
Copy link
Member

deepu105 commented Apr 6, 2022

@pascalgrimaud shall I do the release once this is merged? (using your ansible playbook)

@deepu105
Copy link
Member

deepu105 commented Apr 6, 2022

@atomfrede @pascalgrimaud I have updated the conditions I'll do some tests and merge once CI is green, let me know if you guys have any objections

@atomfrede
Copy link
Member Author

Thanks @deepu105. From my point of view we can do the release. Do we need some additional information for the release notes? I updated the security advisoty description yesterday with more information and how to apply a workaround manually and for what too look in the current code.

@deepu105
Copy link
Member

deepu105 commented Apr 6, 2022

I think we are good. I checked and updated and cleaned up the advisory as well so we just need to merge, release and publish. I'm currently doing some manual tests with this branch.

@atomfrede
Copy link
Member Author

@pascalgrimaud
Copy link
Member

@atomfrede : approved

@DanielFran DanielFran added this to the 7.9.0 milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Injection in Reactive project
4 participants