Detecting / deleting obsolete files not working

[anbraten]

Describe the bug
Delete obsolete is not detecting deleted / non-existing file.

To Reproduce
Steps to reproduce the behavior:

1. Configuration file:

preserve_hierarchy: true

files:
  # Docs Markdown files
  - source: '/test/en/**/*.md'
    translation: '/test/%two_letters_code%/**/%original_file_name%'

2. tree test/:

test/
└── en
    ├── support.md
    └── test.md

2 directories, 2 files

3. Execute CLI command: npx @crowdin/cli upload sources --token ... --project-id ... --delete-obsolete --no-auto-update
   Outputs:

✔️  Fetching project info     
⏭  File 'test/en/support.md'
⏭  File 'test/en/test.md'
✔️  No obsolete files were found
✔️  No obsolete directories found

5. Rename file test/en/support.md => test/en/help.md
6. Execute CLI command again: npx @crowdin/cli upload sources --token ... --project-id ... --delete-obsolete --no-auto-update
   Outputs:

✔️  Fetching project info     
⏭  File 'test/en/test.md'
⏭  File 'test/en/help.md'
✔️  No obsolete files were found
✔️  No obsolete directories found

Expected behavior
I expect the file test/en/support.md to be detected as obsolete and deleted on Crowdin afterwards as it does not exists locally anymore.

Environment:

• OS: Ubuntu
• Version 23.04
• Cli: @crowdin/cli@3.19.2

[anbraten]

Issue seems to be coming from the file exportPattern property being null in the following code section. Why is the export setting checked at all? Shouldn't deleting files only depend on the source path and the path it's saved in crowdin?

crowdin-cli/src/main/java/com/crowdin/cli/commands/functionality/ObsoleteSourcesUtils.java

Line 32 in 2d4d0f0

return exportPattern.equals(fileExportPattern != null ? Utils.normalizePath(fileExportPattern) : null);

[andrii-bodnar]

Hi @anbraten, thanks for your input!

I suppose the exportPattern check is needed in cases where the local file path might be different from the uploaded file path in Crowdin. For example, in case of using preserve_hierarchy: false or dest parameters, the uploaded file path might be different from the original file path. Does this make sense?

[anbraten]

Hi @andrii-bodnar,
thanks for the explanation. So exportPattern will be set in api in case I uploaded a file with preserve_hierarchy: false or dest options configured (couldn't find any explanation in the api docs)? Should I simply ignore in checkExportPattern if the api returned exportPattern is null?

[andrii-bodnar]

CLI will always set the export pattern during the sources upload, even with preserve_hierarchy: false or dest options configured. In most cases, it will be the same as the translation pattern from the configuration file (except the cases with ** usage).

If the exportPattern is null - the file probably wasn't uploaded by the CLI and shouldn't be deleted as well by the --delete-obsolete option.

[anbraten]

If the exportPattern is null - the file probably wasn't uploaded by the CLI and shouldn't be deleted as well by the --delete-obsolete option.

The file you can see in the previous screenshot I've uploaded using the cli first and then debugged a second upload where that file then has exportPattern set to null. Therefore I am not really sure if this statement is correct. Do you have any suggestion how we can do further tests here?

[andrii-bodnar]

@anbraten could you please provide a test project (the crowdin.yml configuration file, corresponding source files structure) and steps to reproduce that I can try on my side?

[anbraten]

@andrii-bodnar Sure, here is a pretty basic sample: https://github.com/anbraten/repro-crowdin-delete-obsolete. Added a readme with some steps what I've done.

[andrii-bodnar]

@anbraten thank you, I was able to reproduce the issue. Could you please take a look at my comment in the PR and check the failed CI? It looks like the new test case is not adapted to work in the Windows environment.