Fix error on writing cache file in the Java client #10642

zambrovski · 2022-10-07T19:53:04Z

Description

Fixed directory creation if the cache file is missing. Added tests for it.

Related issues

closes #10641

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

github-actions · 2022-10-07T20:10:36Z

Test Results

  942 files ±    0   942 suites ±0 1h 48m 16s ⏱️ + 7m 22s
7 732 tests +236 7 725 ✔️ +236 7 💤 ±0 0 ❌ ±0
7 929 runs +242 7 920 ✔️ +242 9 💤 ±0 0 ❌ ±0

Results for commit c2a068d. ± Comparison against base commit cd87f11.

♻️ This comment has been updated with latest results.

korthout · 2022-10-08T08:22:29Z

Hi @zambrovski 👋 Thanks for reporting and immediately fixing it as well! I'll have a good look at the PR after the weekend. I quickly wanted to highlight our error message guidelines.

🔧 Please align the exception message (x must be a directory) with these guidelines

zambrovski · 2022-10-08T09:09:14Z

Will fix the exception text today

korthout

👍 Thanks @zambrovski and sorry that this is taking so long.

💭 Please note that I apply the emoji-code in my reviews.

❌ Please have a look at my change requests below.

❓ I've also manually tested your changes, and I wondered whether the new behavior is what you want.

Previously, having a file or symlink named .camunda in your home folder ~ caused an exception:

Caused by: java.nio.file.FileAlreadyExistsException: /Users/korthout/.camunda

Now, that same scenario leads to an exception:

Caused by: java.io.IOException: Expected /Users/korthout/.camunda to be a directory, but it was a regular file.

👍 IMO, the difference is that the error message is clearer and has a stracktrace that better describes what is happening. Additionally, the code has become more expressive about its assumptions.

❓ That's enough for me, but I would like to verify this behavior with you before we merge it.

❌ If this works for you, please rebase (e.g. squash) your commits into one or multiple commits with meaningful commit messages adhering to our commit message guidelines.

.idea/codeStyles/Project.xml

zambrovski · 2022-10-10T14:02:56Z

You are probably right. It should be even better and just re-use the directory if it is one.. Let me improve this and apply your modifications.

…better error messages and tests, fix #10641

zambrovski · 2022-10-10T18:19:52Z

I hope, I finally got all guidelines fulfilled (Checkstyle, exception texts, git style, commit message style).
I added more test cases and more elaborate handling of what can happen, if the parent doesn't exist(easy) and if it exists and is a file, is a directory, is a symlink-broken, is a symlink-to-file or is symlink-to-dir.

zambrovski · 2022-10-10T18:20:33Z

By the way, is already hacktoberfest?

saig0 · 2022-10-11T04:49:49Z

By the way, is already hacktoberfest?

@zambrovski yes, it is. 🥳

This pull request will count toward the Hacktoberfest challenge. If you contributed four pull requests then you can complete your Camunda Hacktoberfest challenge here.

korthout

Wow thanks @zambrovski That's much more thorough.

👍 I like that the file can now be created if the parent directory already exists or when it's a symlink to a directory. That's much more resilient and easy to use. Cool stuff!

I have one last question before I can accept it. Please have a look.

clients/java/src/test/java/io/camunda/zeebe/client/impl/oauth/OAuthCredentialsCacheTest.java

clients/java/src/main/java/io/camunda/zeebe/client/impl/oauth/OAuthCredentialsCache.java

korthout

Thanks again @zambrovski

LGTM 🏆

bors merge

korthout · 2022-10-13T09:03:54Z

We seem to be having some issues with our merging bot (bors). I don't see a need to get this merged immediately, so let's await the fix.

korthout · 2022-10-13T09:29:47Z

We were able to pinpoint the issue as something related to permissions. A solution is available by rebasing the affected PRs onto main.

@zambrovski Please rebase this PR onto the latest main

zambrovski · 2022-10-19T06:52:24Z

Rebased...

korthout · 2022-10-19T09:18:45Z

@zambrovski c2a068d merges main into this branch. The backport-action is not able to cherry-pick merge commits. Please remove this commit and rebase on top of main instead.

EDIT: Sorry for all this back and forth. If you'd prefer that I take over the last bit to get this merged, then please let me know.

10825: Fix restore app startup and shutdown behavior r=oleschoenburg a=oleschoenburg - fixes injection of backup store - fixes missing shutdown of restore app - fixes a confusing and noisy exception when restore fails within a docker container closes #10824 10831: Fix error on writing cache file in the Java client r=saig0 a=korthout ## Description  This is a recreation of #10642, as I had to rebase the branch. ## Related issues  closes #10641 Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com> Co-authored-by: Simon Zambrovski <simon.zambrovski@holisticon.de>

10831: Fix error on writing cache file in the Java client r=saig0 a=korthout ## Description  This is a recreation of #10642, as I had to rebase the branch. ## Related issues  closes #10641 10833: ci: run go client tests on github hosted runner r=korthout a=oleschoenburg Go client tests should be able to run ob github hosted runners which are more reliable than the `n1-standard-8-netssd-preempt` runners used before. Co-authored-by: Simon Zambrovski <simon.zambrovski@holisticon.de> Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>

zambrovski · 2022-10-28T08:07:36Z

Hi,

sorry for not responding. Since the last one is not functional anymore, feel free to take over and perform the last steps to comply to git rules that you described. I would be happy if you do this, before I do something in a wrong way and have to redo it again.

Cheers,

Simon

korthout · 2022-10-28T09:11:19Z

Thanks @zambrovski. I've opened #10831 for this, and it's being backported to 8.1 and 8.0. Now that it's merged, I'll close this PR. Thanks again for your contribution 🚀

remcowesterhoud requested a review from korthout October 10, 2022 08:35

korthout requested changes Oct 10, 2022

View reviewed changes

.idea/codeStyles/Project.xml Outdated Show resolved Hide resolved

fix(clients/java): implement more resilient cache file handling, add …

4a73b23

…better error messages and tests, fix #10641

zambrovski requested a review from korthout October 11, 2022 06:58

korthout requested changes Oct 11, 2022

View reviewed changes

clients/java/src/test/java/io/camunda/zeebe/client/impl/oauth/OAuthCredentialsCacheTest.java Outdated Show resolved Hide resolved

clients/java/src/main/java/io/camunda/zeebe/client/impl/oauth/OAuthCredentialsCache.java Show resolved Hide resolved

test(clients/java): rename test method to express the test behaviour

2cc920f

korthout added backport stable/8.0 labels Oct 12, 2022

korthout approved these changes Oct 12, 2022

View reviewed changes

saig0 added the hacktoberfest-accepted Marks an issue as having been accepted as a Hacktoberfest contribution label Oct 12, 2022

saig0 changed the title ~~Bugfix/10641 error writing cache file~~ Fix error on writing cache file in the Java client Oct 14, 2022

Merge branch 'main' into bugfix/10641_error_writing_cache_file

c2a068d

korthout mentioned this pull request Oct 27, 2022

Fix error on writing cache file in the Java client #10831

Merged

10 tasks

korthout closed this Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error on writing cache file in the Java client #10642

Fix error on writing cache file in the Java client #10642

zambrovski commented Oct 7, 2022 •

edited by korthout

github-actions bot commented Oct 7, 2022 •

edited

korthout commented Oct 8, 2022

zambrovski commented Oct 8, 2022

korthout left a comment

zambrovski commented Oct 10, 2022

zambrovski commented Oct 10, 2022 •

edited

zambrovski commented Oct 10, 2022

saig0 commented Oct 11, 2022 •

edited

korthout left a comment •

edited

korthout left a comment

korthout commented Oct 13, 2022

korthout commented Oct 13, 2022

zambrovski commented Oct 19, 2022

korthout commented Oct 19, 2022 •

edited

zambrovski commented Oct 28, 2022

korthout commented Oct 28, 2022

Fix error on writing cache file in the Java client #10642

Fix error on writing cache file in the Java client #10642

Conversation

zambrovski commented Oct 7, 2022 • edited by korthout

Description

Related issues

Definition of Done

github-actions bot commented Oct 7, 2022 • edited

Test Results

korthout commented Oct 8, 2022

zambrovski commented Oct 8, 2022

korthout left a comment

Choose a reason for hiding this comment

zambrovski commented Oct 10, 2022

zambrovski commented Oct 10, 2022 • edited

zambrovski commented Oct 10, 2022

saig0 commented Oct 11, 2022 • edited

korthout left a comment • edited

Choose a reason for hiding this comment

korthout left a comment

Choose a reason for hiding this comment

korthout commented Oct 13, 2022

korthout commented Oct 13, 2022

zambrovski commented Oct 19, 2022

korthout commented Oct 19, 2022 • edited

zambrovski commented Oct 28, 2022

korthout commented Oct 28, 2022

zambrovski commented Oct 7, 2022 •

edited by korthout

github-actions bot commented Oct 7, 2022 •

edited

zambrovski commented Oct 10, 2022 •

edited

saig0 commented Oct 11, 2022 •

edited

korthout left a comment •

edited

korthout commented Oct 19, 2022 •

edited