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.

Sign up for GitHub

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

Jump to bottom

chore(engine, engine-rest): Add CreateTime Flag to Fetch and Lock API #3912

Merged
merged 22 commits into from
Dec 13, 2023
Merged

chore(engine, engine-rest): Add CreateTime Flag to Fetch and Lock API #3912

merged 22 commits into from
Dec 13, 2023

Conversation

psavidis
Copy link
Contributor

@psavidis psavidis commented Nov 3, 2023 •
edited
Loading

  • Fetch & Lock API Changes

Related-to: #3896

@psavidis psavidis self-assigned this Nov 3, 2023
@psavidis psavidis changed the title 3896 fetch and lock chore(engine, engine-rest): Add CreationDate Flag to Fetch and Lock API Nov 3, 2023
@psavidis psavidis mentioned this pull request Nov 3, 2023
Closed
1 task
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from f8b516f to 2f9d5cf Compare November 3, 2023 17:45
@psavidis psavidis force-pushed the 3896-persistence-layer-adjustments branch from 1b98f18 to 70c15f8 Compare November 6, 2023 16:43
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch 2 times, most recently from 28c90fd to 9eb9fe4 Compare November 6, 2023 17:18
@psavidis psavidis changed the title chore(engine, engine-rest): Add CreationDate Flag to Fetch and Lock API chore(engine, engine-rest): Add CreateTime Flag to Fetch and Lock API Nov 6, 2023
@psavidis psavidis requested a review from mboskamp November 6, 2023 17:20
@psavidis psavidis marked this pull request as ready for review November 7, 2023 07:20
@psavidis psavidis added the ci:all-db Runs the builds for all databases. label Nov 7, 2023
@psavidis psavidis force-pushed the 3896-persistence-layer-adjustments branch 3 times, most recently from 99cb039 to e278542 Compare November 7, 2023 22:28
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from 6edea26 to ccf8675 Compare November 7, 2023 22:36
@psavidis psavidis mentioned this pull request Nov 8, 2023
Closed
@psavidis
chore(engine): Adjust Persistence Layer for CreationDate
73953bd 
- MyBatis Adjustments
- ExternalTask is saved & Creation Date is persisted
- orderByCreationDate works with ASC & DESC Order
- HistoryExternalTaskLog uses creation date
@psavidis
test(model-api): Add Tests for Multi-Level Sorting
a0d3ac6 
Creation-Date & Priority
@psavidis
refactor(engine): Rename CREATION_DATE to CreateTime
6a8919c
@psavidis psavidis force-pushed the 3896-persistence-layer-adjustments branch from e278542 to 6a8919c Compare November 9, 2023 16:06
@psavidis
chore(engine): Adjust Fetch And Lock API
056b746 
- Add parameter useCreationDate to ExternalTaskService
- The parameter passes-through until FetchExternalTasksCmd where it is not currently utilized
@psavidis
tests(engine): Adjust Tests
8987e7f 
Adjust test usage of externalTaskService#fetchAndLock to use `useCreationDate` with false value
@psavidis
chore(engine-rest): Adjust Fetch & Lock API
246423f 
- Configuring creationDate fro ExternalTaskService#fetchAndLock is performed using `CreationDateConfig` enum
- ExternalTaskRestService uses `CreationDateConfig` DTO to pass direction and enablement /disablement of `useCreationDate` param
- Unit Tests for creationDate DESC, ASC order using ExternalTaskService

- Pending are the multi-level sorting tests for both priority and creationDate
@psavidis
chore(engine): Add Multi-Level Sorting Tests
f552e14 
- Remove Tests and rewrite them:

testFetchWithCreationDateASC
testFetchWithCreationDateDESC

Replaced by:

shouldFetchWithCreationDateDESCWithoutPriority
shouldFetchWithCreationDateASCWithoutPriority

- Externalise CreationDate to `LockedExternalTaskImpl`
@psavidis
test(engine): Add Tests for Null Or Empty Configs
ab748fc
@psavidis
Rename CreationDate to CreateTime
a10d35d
@psavidis
refactor(engine): Fix Tests for null & Empty createTimeConfig
05e9de6
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from 5d7a359 to 05e9de6 Compare November 9, 2023 16:11
mboskamp
mboskamp requested changes Nov 10, 2023
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

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

As discussed: There already is a generic sorting mechanism for query APIs that we can use for the fetch and lock sorting as well.

...e-rest/src/main/java/org/camunda/bpm/engine/rest/dto/externaltask/FetchExternalTasksDto.java Outdated Show resolved Hide resolved
...e-rest/src/main/java/org/camunda/bpm/engine/rest/dto/externaltask/FetchExternalTasksDto.java Outdated Show resolved Hide resolved
...e-rest/src/main/java/org/camunda/bpm/engine/rest/dto/externaltask/FetchExternalTasksDto.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/Direction.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/ExternalTaskServiceImpl.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/ExternalTaskServiceImpl.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from 021e508 to c7df2d7 Compare November 14, 2023 16:29
@psavidis
chore(engine, engine-rest): Replace CreateTimeConfig with Builder
5033882 
- Apply Code Review points
- Use Sortings for fetch And Lock API
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from c7df2d7 to 5033882 Compare November 15, 2023 07:33
@psavidis
chore(engine, engine-rest): Adjust ExternalTaskServiceTest tests
c4832e2
@psavidis
chore(engine, engine-rest): Refactor builder
382800e 
Refactor FetchAndLockBuilder not to extend ExternalTaskQueryTopicBuilder but rather return it
@psavidis
chore(engine, engine-rest): Fix Tests
417c251 
- Fix constructors to initialize internally dependencies that do not belong outside their scope
- ExternalTaskQueryTopicBuilderImpl copy constructor copies currentInstruction
@psavidis
chore(engine, engine-rest): Add CreateTime Tests to REST API
8f1555e 
- Add tests that verify the parameter propagation from the DTO to the builder and the correct invocation
- Add validations to the FetchAndLockBuilderImpl similar to the QueryAPI for ensuring the correct method invocation on the sorting field and the order
@psavidis psavidis requested a review from tasso94 November 20, 2023 08:09
mboskamp
mboskamp requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

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

This already looks very good. I added some smaller hints. I have one more concern about the design of the external task builders that I would like to talk to you about synchronously if you are up for it. I left some comments already, but only on some affected lines. I also did not look too much at the tests yet.

engine/src/main/java/org/camunda/bpm/engine/ExternalTaskService.java Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/externaltask/FetchAndLockBuilderImpl.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
.../src/main/java/org/camunda/bpm/engine/impl/history/producer/DefaultHistoryEventProducer.java Show resolved Hide resolved
.../src/main/java/org/camunda/bpm/engine/impl/history/producer/DefaultHistoryEventProducer.java Show resolved Hide resolved
...e-rest/src/test/java/org/camunda/bpm/engine/rest/ExternalTaskRestServiceInteractionTest.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from 95ae02c to d130961 Compare November 27, 2023 09:23
@mboskamp mboskamp mentioned this pull request Nov 27, 2023
Merged
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from 3707b3a to 876c970 Compare November 27, 2023 14:40
@psavidis psavidis requested a review from mboskamp November 30, 2023 07:32
@psavidis psavidis requested a review from tasso94 November 30, 2023 14:59
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch 2 times, most recently from 3818ff3 to cea05c6 Compare December 1, 2023 14:23
@psavidis
refactor(engine): Code Review points
c50657a 
- Remove execute method from FetchAndLockBuilder
- Refactor FetchAndLockBuilderImpl to use a List instead of a Map and make it similar to the QueryAPI for the sake of consistency
- Add tests for covering invalid usages of fetchAndLock API
- Infer useCreateTime
- Correct JavaDoc
- Generify Sorting of FetchAndLock Impl
- Remove var
@psavidis psavidis force-pushed the 3896-fetch-and-lock branch from cea05c6 to c50657a Compare December 1, 2023 14:57
mboskamp
mboskamp requested changes Dec 11, 2023
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

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

Thanks for your effort. I only spotted some minor things. Nice job implementing all the suggestions so far.

...e-rest/src/main/java/org/camunda/bpm/engine/rest/dto/externaltask/FetchExternalTasksDto.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/externaltask/FetchAndLockBuilderImpl.java Outdated Show resolved Hide resolved
engine/src/main/java/org/camunda/bpm/engine/impl/persistence/entity/ExternalTaskManager.java Outdated Show resolved Hide resolved
...t/src/test/java/org/camunda/bpm/engine/rest/impl/FetchAndLockRestServiceInteractionTest.java Outdated Show resolved Hide resolved
...e-rest/src/test/java/org/camunda/bpm/engine/rest/ExternalTaskRestServiceInteractionTest.java Show resolved Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/api/externaltask/ExternalTaskServiceTest.java Show resolved Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/api/externaltask/ExternalTaskServiceTest.java Outdated Show resolved Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/api/externaltask/ExternalTaskServiceTest.java Outdated Show resolved Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/api/externaltask/ExternalTaskServiceTest.java Outdated Show resolved Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/api/externaltask/ExternalTaskServiceTest.java Outdated Show resolved Hide resolved
@psavidis
Fix Code Review Points
21a47c1 
- Enrich comments
- Use AssertJ Sort Asertion
- Move when / then blocks
- Reset Clock on TearDown
- Apply Suggestions
- Remove unnecessary method
- Refactor
- Comments
@psavidis
Move when blocks for ExternalTaskQueryByCreateTimeTest
b69bfde
@psavidis psavidis requested a review from mboskamp December 12, 2023 09:40
@psavidis
chore(engine): Cover Code Review Points
93143b3 
- Enhancements in ExternalTaskManager
- Remove usePriority instance field
- Enhance orderingProperties check
mboskamp
mboskamp approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀 Great job @psavidis! This is a nice feature.

@psavidis psavidis merged commit 159d401 into 3896-feature Dec 13, 2023
2 of 4 checks passed
@psavidis psavidis deleted the 3896-fetch-and-lock branch December 13, 2023 16:36
@psavidis psavidis mentioned this pull request Apr 8, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboskamp mboskamp mboskamp approved these changes

@tasso94 tasso94 Awaiting requested review from tasso94

Assignees

@psavidis psavidis

Labels
ci:all-db Runs the builds for all databases.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@psavidis @tasso94 @mboskamp