-
Notifications
You must be signed in to change notification settings - Fork 2
Feature/281 business key support #306
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @klajmajk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements the highly anticipated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a 'business key' feature to the BPMN engine, allowing process instances to be correlated with external business entities. The changes involve updating the database schema to include a business_key column in the process_instance table, modifying internal data structures (bpmnruntime.ProcessInstance, sql.ProcessInstance, proto.ProcessInstance) to store and retrieve this key, and extending the API to support creating process instances with a business key and querying instances by it. Specifically, new appcontext functions (WithBusinessKey, BusinessKeyFromContext) were added, CreateInstance and GetProcessInstances methods in node.go and server.go were updated, and the OpenAPI specification was extended to reflect these new API capabilities. Review comments highlighted an incorrect CASE statement for string filtering in SQL queries, suggesting a NULL check instead of != 0, and pointed out commented-out test code for temporary directory cleanup and a businessKey assertion that should be re-enabled after API fixes. Additionally, a suggestion was made to use url.QueryEscape for businessKey in test URLs to handle special characters robustly.
| CASE WHEN @business_key <> 0 THEN | ||
| process_instance.business_key = @business_key | ||
| ELSE | ||
| 1 | ||
| END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CASE statement for filtering by business_key is incorrect. The condition WHEN @business_key <> 0 will not work as intended for string values in SQLite, as non-numeric strings are coerced to 0 during comparison. This will cause filtering by a non-numeric business_key to fail.
A more robust way to handle optional string filters is to check for NULL.
CASE WHEN @business_key IS NOT NULL THEN
process_instance.business_key = @business_key
ELSE
1
END
test/e2e/e2e_suite_test.go
Outdated
| } | ||
| openTelemetry.Stop(appContext) | ||
| os.RemoveAll(tempDir) | ||
| // os.RemoveAll(tempDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/e2e/process_instance_test.go
Outdated
| }) | ||
| assert.NoError(t, err) | ||
| assert.NotEmpty(t, instance.Key) | ||
| // assert.NotEmpty(t, instance.BusinessKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is commented out, likely because the CreateProcessInstance endpoint doesn't return the businessKey. Once the API response is fixed, this assertion should be re-enabled to ensure the businessKey is correctly returned upon creation.
| // assert.NotEmpty(t, instance.BusinessKey) | |
| assert.NotEmpty(t, instance.BusinessKey) |
|
|
||
| func getProcessInstances(t testing.TB, key string) ([]public.ProcessInstance, error) { | ||
| resp, err := app.NewRequest(t). | ||
| WithPath(fmt.Sprintf("/v1/process-instances?businessKey=%s", key)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The businessKey is being inserted directly into the URL string. This could lead to issues if the key contains special characters that need to be URL-encoded. Using url.QueryEscape would make this test helper more robust.
You'll also need to import the net/url package.
| WithPath(fmt.Sprintf("/v1/process-instances?businessKey=%s", key)). | |
| WithPath(fmt.Sprintf("/v1/process-instances?businessKey=%s", url.QueryEscape(key))). |
Adds business key support