Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Improved behavioral consistency between the database services #837

Merged
merged 4 commits into from
Oct 10, 2024
Merged

fix: Improved behavioral consistency between the database services #837

merged 4 commits into from
Oct 10, 2024

Conversation

BobdenOs
Copy link
Contributor

This PR contains a few fixes that where required to make the new compliance tests to pass:

  • Decimal precision behavior is aligned between the databases when defined
  • @cap-js/hana now properly supports scalar SELECT queries in the columns

The goal of this PR is to remove as much dependencies as possible now that cds@8 provides the cds-test command.

  • Removing jest by switching to cds-test
  • Removed jest.config.js files
  • Removing chai by switching to cds-test
  • Adjust all tests to fit with the cds-test expect pattern
  • Removing HANA test action
  • Move HXE Github action steps into a reuse file
  • Added a lock for HANA database creation
  • Added a fallback for Postgres during database creation
  • Removed compliance import tests
  • Added symbolic link to the compliance test folder

BobdenOs and others added 2 commits October 10, 2024 06:39
@BobdenOs @danjoa @johannes-vogel
fix: Improved behavioral consistency between the database services (#673
26c4584 
)

This PR contains a few fixes that where required to make the new
compliance tests to pass:
- Decimal precision behavior is aligned between the databases when
defined
- `@cap-js/hana` now properly supports scalar `SELECT` queries in the
columns

The goal of this PR is to remove as much dependencies as possible now
that `cds@8` provides the `cds-test` command.

- Removing `jest` by switching to `cds-test`
- Removed `jest.config.js` files
- Removing `chai` by switching to `cds-test`
- Adjust all tests to fit with the `cds-test` `expect` pattern
- Removing `HANA` test action
- Move `HXE` Github action steps into a reuse file
- Added a lock for `HANA` database creation
- Added a fallback for `Postgres` during database creation
- Removed compliance import tests
- Added symbolic link to the compliance test folder

---------

Co-authored-by: Daniel Hutzel <daniel.hutzel@sap.com>
Co-authored-by: Johannes Vogel <31311694+johannes-vogel@users.noreply.github.com>
@BobdenOs
Fix test errors for newer tests and older node version
fa50efc
@BobdenOs BobdenOs changed the title fix: Improved behavioral consistency between the database services #836 fix: Improved behavioral consistency between the database services Oct 10, 2024
@johannes-vogel johannes-vogel mentioned this pull request Oct 10, 2024
Closed
@BobdenOs BobdenOs merged commit b6f7187 into main Oct 10, 2024
3 checks passed
@BobdenOs BobdenOs deleted the clean-dependencies-2 branch October 10, 2024 13:08
@cap-bots cap-bots mentioned this pull request Oct 10, 2024
Merged
@patricebender patricebender mentioned this pull request Oct 11, 2024
Merged
patricebender added a commit that referenced this pull request Oct 11, 2024
@patricebender
fix: respect user provided alias of subquery in from (#826)
c6e5e0b 
the fix was part of #837 → Semantic commit indicates a fix so that this
gets mentioned in the changelog

this fixes cap/issue#16943
@johannes-vogel johannes-vogel mentioned this pull request Oct 14, 2024
Merged
patricebender pushed a commit that referenced this pull request Oct 14, 2024
@johannes-vogel
fix: usage of between operator (#841)
b37082a 
closes #812

issue was actually fixed with #837
@sebastien-savalle
Copy link

Can you please report this fix to a a version which support cds 7 ?

Actually, there are some inconsistencies:

  • Latest version of the db-service still support cds 7.9
  • cds-hana requires at least cds 8.2

So, when you fix a bug (like in this PR) which involves both dependencies, only project using cds > 8.2 can benefit from the fix.

@johannes-vogel
Copy link
Contributor

Can you please report this fix to a a version which support cds 7 ?

Actually, there are some inconsistencies:

  • Latest version of the db-service still support cds 7.9
  • cds-hana requires at least cds 8.2

So, when you fix a bug (like in this PR) which involves both dependencies, only project using cds > 8.2 can benefit from the fix.

I do not see any inconsistencies.
There are parts in @cap-js/hana which require a newer version even though the base package @cap-js/db-service does not need this. The base package is also used for sqlite and postgres where we do not yet require cds8.
In addition, @cap-js/hana is officially supported with cds8.

Are you running on cds7 + cap-js/hana? If yes, please consider updating the cds version.

@sebastien-savalle
Copy link

sebastien-savalle commented Oct 14, 2024 •
edited
Loading

No, we are running with cds7 and cap-js/hana, but we would like to first migrate to cap-js/hana (which is not easy) and then to cds8 to avoid too many regressions.
Why did you publish a final version (1.0 & 1.1) of @cap-js/hana if the support of cds-7 was not official ?

@johannes-vogel
Copy link
Contributor

No, we are running with cds7 and cap-js/hana, but we would like to first migrate to cap-js/hana (which is not easy) and then to cds8 to avoid too many regressions.

We do recommend to first upgrade to cds8. You can adopt that and turn off the new odata protocol adapter. With that, there should not be much upgrade effort. Then you can adopt @cap-js/hana and later on the new protocol adapter or vice versa.

Why did you publish a final version (1.0 & 1.1) of @cap-js/hana if the support of cds-7 was not official ?

There simply was no technical reason to restrict it to cds8 before.

johannes-vogel added a commit that referenced this pull request Oct 15, 2024
@cap-bots @johannes-vogel
chore: release main (#835)
9530ee4 
🤖 I have created a release *beep* *boop*
---


<details><summary>db-service: 1.14.0</summary>

##
[1.14.0](db-service-v1.13.0...db-service-v1.14.0)
(2024-10-15)


### Added

* assoc-like calc elements after exists predicate
([#831](#831))
([05f7d75](05f7d75))


### Fixed

* Improved behavioral consistency between the database services
([#837](#837))
([b6f7187](b6f7187))
* Treat assoc-like calculated elements as unmanaged assocs
([#830](#830))
([cbe0df7](cbe0df7))
</details>

<details><summary>sqlite: 1.7.4</summary>

##
[1.7.4](sqlite-v1.7.3...sqlite-v1.7.4)
(2024-10-15)


### Fixed

* Improved behavioral consistency between the database services
([#837](#837))
([b6f7187](b6f7187))
</details>

<details><summary>postgres: 1.10.1</summary>

##
[1.10.1](postgres-v1.10.0...postgres-v1.10.1)
(2024-10-15)


### Fixed

* add cds schema for postgres build plugin
([#843](#843))
([6306d5c](6306d5c))
* Improved behavioral consistency between the database services
([#837](#837))
([b6f7187](b6f7187))
* null as default value
([#845](#845))
([0041ec0](0041ec0))
</details>

<details><summary>hana: 1.3.1</summary>

##
[1.3.1](hana-v1.3.0...hana-v1.3.1)
(2024-10-15)


### Fixed

* combination of groupby and orderby
([#825](#825))
([10e0534](10e0534))
* Improved behavioral consistency between the database services
([#837](#837))
([b6f7187](b6f7187))
* list optimization for `Buffer` values
([#817](#817))
([3e60de2](3e60de2))
* null as default value
([#845](#845))
([0041ec0](0041ec0))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: Johannes Vogel <johannes.vogel@sap.com>
@johannes-vogel johannes-vogel mentioned this pull request Oct 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannes-vogel johannes-vogel johannes-vogel approved these changes

@patricebender patricebender Awaiting requested review from patricebender patricebender is a code owner

@gregorwolf gregorwolf Awaiting requested review from gregorwolf gregorwolf is a code owner

@sebastianesch sebastianesch Awaiting requested review from sebastianesch sebastianesch is a code owner

@vobu vobu Awaiting requested review from vobu vobu is a code owner

@danjoa danjoa Awaiting requested review from danjoa danjoa is a code owner

@stewsk stewsk Awaiting requested review from stewsk stewsk is a code owner

@sjvans sjvans Awaiting requested review from sjvans sjvans is a code owner

@David-Kunz David-Kunz Awaiting requested review from David-Kunz David-Kunz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BobdenOs @sebastien-savalle @johannes-vogel