@sap/cds 6.5.0 compatibility

[ryegrosT8]

Minor changes to allow @sap/cds 6.5.0 compatibility.

[gregorwolf]

Hi @ryegrosT8,

thank you for this great step. And the tests still work! Will give it a try later with @sap/cds 6.5.0. I hope you are also aware of: cds-pg → CAP community core #363.

CU
Gregor

[ryegrosT8]

Hi @gregorwolf ,

I wasn’t aware of #363, that is really good news. We are right now in the middle of a medium size app for BTP and some of managers wasn’t sure about the initial decision to use cds-pg.
I will share the discussion with them :) .

Regards.
Rafael Yegros.

[gregorwolf]

Hi @ryegrosT8,

there is a bright future for cds-pg and your company made a good decision to share the improvements and make the already available module better.

Best Regards
Gregor

[ryegrosT8]

Thank you very much for the words @gregorwolf.

I tried to use the changes on this PR in my company project and got 2 more fix points.

1 - Seems like "_ensureModel" was removed from @sap/cds/libx/_runtime/db/Service.js, I had to locally implement it on "PostgresDatabase" Service and to use a different strategy since in some cases was not possible anymore to acquire the "model" from "req" object. I choose to look up for model definition at ServiceApplication instance by:

/**
   * Retrieve model definition from ApplicationService
   */
  _ensureModel(){
    for(const service of cds.services){
      if(service instanceof cds.ApplicationService){
        this.model = service.model
        break;
      }         
    }
  }

Not sure if it's the best approach (maybe it become a problem when try to support multitenancy), but it works for now.

2 - The handler for Virtual fields was acting a little weirdo, I had to did a "bind(this)" on "this._virtual" at handler declaration to put the virtual fields tests class up and running again.

    this.before('READ', '*', this._virtual.bind(this));

A internal reference to “this” in @sap/cds/libx/_runtime/db/generic/virtual.js could be the problem.

PS. I tested on my project and looks like everything is working well now. If this PR got promoted, I will propose other at cds-dbm, seems like equivalent changes need to be done there.

Best Regards.
Rafael Yegros.

[gregorwolf]

Hi @ryegrosT8,

can you check your editor settings as there are many changes that removed just a , or added / removed whitespace.

Regarding your additional findings: If they are not covered yet by unit tests it would be great if you could add them. If there is nothing we can do in the module regarding the virtual fields then we should mention that in the readme. But maybe @danjoa could point someone from the CAP team here to get this sorted.

CU
Gregor

[vobu]

just pinging the round here to monitor any changes in the tests...i will then port them over to the new cds-pg home :)

[ryegrosT8]

I agree, some changes was only indent and comma at line end.

Actually my editor was following the .prettierrc.js setup at root folder, there the "trailingComma" was set to 'none'. I changed it to 'always' and change some indent settings to match what I saw in the sources that I worked. Would be a good ideia to run a full prettier on project (there is any bot doing it?).

Regarding the virtual field, just to be clear, look like it's working well with the "bind(this)". But would be good to check it on @sap/cds, indeed.

About the test's of the last two fixes. For the first, I was only able to catch it only in my own application that is a bit more complex than Beer Shop. (TypeScript, cds2types, cds-routing-handlers and cds-dbm). Actually, I was really surprised that the current test cases don’t break, probably because we had in “PostgresDatabase” implementation a:

this._ensureModel && this.before('*', this._ensureModel)

that was hiding the absence of the _ensureModel implementation, otherwise we will got an exception on any test case that call the "handler('*')" execution, since _ ensureModel would be 'undefined', right?

I removed the "&&" operator to let us know if the "_ensureModel" implementation becomes a problem in future.

For the second fix, after the first fix, the current test case for virtual fields start to cover the error.

PS. Please feel free to point if I did something out of expected, I'm not used to contribute to open-source projects like this, actually this is the first time.

Best regards.
Rafael Yegros.

[ryegrosT8]

Hi there,

Any news here? Is there anything else that I should do?

I also created a fork on cds-dbm with for the upgrade of @sap/cds too (https://github.com/ryegrosT8/cds-dbm), I'm just waiting the conclusion here to update the cds-pg lib on my cds-dbm fork and submit it as pull request.

Best regards.
Rafael Yegros.

[vobu]

just two very small things, then this is good to go! excellent!