Skip to content
This repository has been archived by the owner on Jul 14, 2023. It is now read-only.

@sap/cds 6.5.0 compatibility #383

Merged
merged 16 commits into from
Feb 13, 2023
Merged

@sap/cds 6.5.0 compatibility #383

merged 16 commits into from
Feb 13, 2023

Conversation

ryegrosT8
Copy link
Contributor

Minor changes to allow @sap/cds 6.5.0 compatibility.

@gregorwolf
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Collaborator

vobu commented Feb 1, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Collaborator

@vobu vobu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.prettierrc.js Outdated Show resolved Hide resolved
lib/pg/Service.js Outdated Show resolved Hide resolved
@vobu vobu merged commit 28b2c42 into sapmentors:main Feb 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants