Improvement suggestions

[maginseb]

The PR contains some improvement suggestions.

1. Adds the missing dev-dependency to @sap/cds-dk in the db module.
   This fixes a problem that causes the build command to fail with could not determine executable to run

2. Adds an example route mapping to README.md

Removed based on the communication.
Fixes an issue in the Multi Tenancy example caused by a missing class.
Adds org.apache.commons:commons-collections4:4.4 as dependency. The dependency is required for com.sap.cds:cds-feature-mt:3.4.0 but never imported.
This is likely a bug in cds, but the import works around it.

[cla-assistant]

All committers have signed the CLA.

[cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mofterdinger]

Hi Sebastian, thanks for your contribution. See my comments below:

1. Fixes an issue in the Multi Tenancy example caused by a missing class.
   Adds org.apache.commons:commons-collections4:4.4 as dependency. The dependency is required for com.sap.cds:cds-feature-mt:3.4.0 but never imported.
   This is likely a bug in cds, but the import works around it.

This issue is fixed with CAP Java 3.4.1 which is now consumed by this sample application. This fix is no longer required.

2. Adds the missing dev-dependency to @sap/cds-dk in the db module.
   This fixes a problem that causes the build command to fail with `could not determine executable to run`

Can you explain how I can reproduce this build error ? Locally I'm using mvn clean install or mvn spring-boot:run and the build works fine. Also the mbt build works for me. Not sure which issue you try to fix here.

Thanks,
Markus

[maginseb]

Hi Markus,

Thank you for your quick response.

This issue is fixed with CAP Java 3.4.1 which is now consumed by this sample application. This fix is no longer required.

I have removed the workaround from the PR.

Can you explain how I can reproduce this build error ? Locally I'm using mvn clean install or mvn spring-boot:run and the build works fine. Also the mbt build works for me. Not sure which issue you try to fix here.

It could be that you have installed the library @sap/cds-dk globally on your device. If you want to reproduce the problem you can uninstall it and run mbt build.

Best regards,
Sebastian

[mofterdinger]

It could be that you have installed the library @sap/cds-dk globally on your device. If you want to reproduce the problem you can uninstall it and run mbt build.

yes, without a globally install @sap/cds-dk I can reproduce, but it's a prerequisite to install it globally: https://github.com/SAP-samples/cloud-cap-samples-java?tab=readme-ov-file#prerequisites

This is also the default, if you would add HANA support with cds add hana:

{
  "name": "deploy",
  "dependencies": {
    "hdb": "^0",
    "@sap/hdi-deploy": "^5"
  },
  "engines": {
    "node": "^22.0.0"
  },
  "scripts": {
    "start": "node node_modules/@sap/hdi-deploy/deploy.js --use-hdb",
    "build": "npm i && npx cds build .. --for hana --production"
  }
}

[maginseb]

yes, without a globally install @sap/cds-dk I can reproduce, but it's a prerequisite to install it globally: https://github.com/SAP-samples/cloud-cap-samples-java?tab=readme-ov-file#prerequisites

This is also the default, if you would add HANA support with cds add hana:

You are absolutely correct. My setup is somewhat unconventional since I installed cds using Homebrew. As a result, the npx cds command only functions properly for me when @sap/cds-dk is added as a dev-dependency. I will definitely adjust my local setup later.

The reason I initially thought that @sap/cds-dk was missing as a dev-dependency is because the mtx/sidecar module actually includes it. This was introduced in #349. I believe it could be beneficial to align both use cases for consistency.

Best regards,
Sebastian

[beckermarc]

I think this is a valid suggestion, as we also do it like that in mtx/sidecar as already stated. I'd prefer to avoid these dev dependencies, but seems like at the moment they are required, as an effect of our efforts to keep the root package.json free. I guess we would also run into issues otherwise if we would run a db build in a pipeline setup, where no global cds-dk is installed.

It is not ideal for sure, but so far it seems like we haven't found a better solution that doesn't also have some other weird side effects.

[beckermarc]

Does this example really help? What is the additional value/information you want to convey with that?

I am always a little hesitant to have such CLI snippets here, as they are usually copied without much thinking and then people are confused, if they don't work.

[maginseb]

You are right that examples bear the risk to be mindlessly copied. However, for someone like me who wasn't entirely sure what I was looking for, seeing a real example was incredibly helpful.

In this instance, it helped me to see the expected format of <YOUR DOMAIN>. I found this information here.

I can try to include some more information about the assumptions for the example or where to find the respective information. If you think it's not valuable I can also remove it from the PR.

[beckermarc]

okay, maybe it would be better in that case to provide a description where to obtain the value for <YOUR DOMAIN>, e.g.:

Suggested change

	- Example: `cf map-route bookshop cfapps.us10.hana.ondemand.com --hostname subscriber1-myOrg-mySpace-bookshop-mt-app`
	- `<YOUR DOMAIN>`: Find the app domain for your landscape by executing `cf domains`. It commonly starts with `cfapps.`.

[maginseb]

Please have another look on the change. I have followed your suggestion and replaced the example with a description for the parameters that I struggled with during my setup.

[beckermarc]

thanks 👍