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

Make AssociationHelper take DynamicColumns into consideration #4608

Merged
merged 3 commits into from
Sep 14, 2024
Merged

Make AssociationHelper take DynamicColumns into consideration #4608

merged 3 commits into from
Sep 14, 2024

Conversation

boginw
Copy link
Contributor

@boginw boginw commented Aug 28, 2024

This PR refers to #4602 where I mentioned an issue when using a combination of a custom IMetadataReader and DynamicColumnsStore to hold association keys. The issue being that AssociationHelper didn't take Dynamic Columns into account. I hope that this solves the issue.

N.B: Since I'm running Ubuntu, I am unable to run the test to completion (even with Mono). However, with the use of Mono, I can run everything that doesn't talk directly to a database. Before the fix in AssociationHelper.cs the test fails with:

Association key 'ParentID' not found for type 'Tests.Linq.DynamicColumnsTests+DynamicChild

And after the fix it fails with (on my machine):

OleDb is not implemented

This seems to confirm that the issue in #4602 is gone.

Any help confirming that everything else still works properly would be greatly appreciated! :)

@boginw
Copy link
Contributor Author

boginw commented Sep 5, 2024

I was able to use the NuGet artifacts from the CI pipeline to confirm that this does fix the issue I was experiencing :)

@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu MaceWindu added this to the 6.0.0-preview.2 milestone Sep 7, 2024
@MaceWindu
Copy link
Contributor

/azp run test-all

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@linq2dbot
Copy link

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

@linq2dbot linq2dbot mentioned this pull request Sep 10, 2024
Merged
@boginw
Copy link
Contributor Author

boginw commented Sep 11, 2024

Alright, tests failing... I downloaded the test_artifacts and tried emulating the CI/CD to run both the tests for Oracle18 and then for ClickHouse MySql.

I could not reproduce the Oracle18 issue on my local machine. I tried running all tests with this provider 10 times, but it all passed every time. My guess is that the test is flaky or that I have done something differently in my setup. What would be the right course of action here?

As for ClickHouse MySql, running it locally gave me the same result as the CI/CD pipeline. Just for sanity's sake, I wanted to make sure that this test was not already failing on the master branch, so I tried packaging the test artifacts manually using a Windows VM and then running the tests again, and the same test seems to fail too on the master branch. Oddly, when I run the failing test in isolation, in Rider with the same provider, it passes just fine. I see that there is a test-all run for #4595 where it passes. Has this been addressed there?

@MaceWindu
Copy link
Contributor

Ignore those failures:

  • oracle fails randomly due to provider bug
  • clickhouse test fails due to changes to recent ClickHouse release

@MaceWindu
Copy link
Contributor

Yes, #4595 contains fixes for recent CI issues

@boginw
Copy link
Contributor Author

boginw commented Sep 11, 2024

Nice, thanks for the quick response :) Are there any blockers for merging?

@MaceWindu
Copy link
Contributor

Yes, we currently work on some preview.1 regressions I want to merge first. After that I will merge this one

@MaceWindu
Copy link
Contributor

On the other side - I probably will see if I can merge this during weekend

@boginw
Copy link
Contributor Author

boginw commented Sep 11, 2024

Sounds great, much appreciated :)

@MaceWindu
Copy link
Contributor

Thanks!

@MaceWindu MaceWindu merged commit 6e12a13 into linq2db:master Sep 14, 2024
64 of 67 checks passed
MaceWindu pushed a commit to linq2db/linq2db.baselines that referenced this pull request Sep 14, 2024
@linq2dbot
Baselines for linq2db/linq2db#4608 (#1273)
f0fe213 
* [Windows / SQLite (both providers)] baselines

* [Windows / SQLite (specialized tests)] baselines

* [Windows / SQL CE] baselines

* [Windows / Access MDB (Jet/ODBC)] baselines

* [Windows / SQL Server 2005] baselines

* [Windows / SQL Server 2019] baselines

* [Windows / SQL Server 2017] baselines

* [Linux / Firebird 2.5] baselines

* [Windows / SQL Server 2016] baselines

* [Windows / SQL Server 2014] baselines

* [Windows / SQL Server EXTRAS] baselines

* [Linux / Firebird 3.0] baselines

* [Linux / DB2 LUW 11.5] baselines

* [Windows / SQL Server 2008] baselines

* [Windows / SQL Server 2012] baselines

* [Linux / MariaDB 11] baselines

* [Linux / Informix 14.10] baselines

* [Linux / Firebird 5.0] baselines

* [Linux / Firebird 4.0] baselines

* [Linux / ClickHouse Client] baselines

* [Linux / Oracle 11g XE] baselines

* [Linux / MySQL 9 (both providers)] baselines

* [Linux / MySQL 5.7 (both providers)] baselines

* [Windows / SQL Server 2022] baselines

* [Linux / PostgreSQL 12] baselines

* [Linux / Oracle 12c] baselines

* [Linux / PostgreSQL 14] baselines

* [Linux / PostgreSQL 13] baselines

* [Linux / Oracle 23c] baselines

* [Linux / PostgreSQL 11] baselines

* [Linux / PostgreSQL 15] baselines

* [Linux / PostgreSQL 16] baselines

* [Linux / Oracle 21c] baselines

* [Linux / Oracle 19c] baselines

* [Linux / ClickHouse Octonica] baselines

* [Linux / Sybase ASE 16] baselines

* [Linux / SQL Server 2019] baselines

* [Linux / SAP HANA 2] baselines

---------

Co-authored-by: Azure Pipelines Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaceWindu MaceWindu MaceWindu approved these changes

@igor-tkachev igor-tkachev Awaiting requested review from igor-tkachev igor-tkachev is a code owner

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

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

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

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

Assignees
No one assigned
Labels
None yet
Milestone
6.0.0-preview.2
Development

Successfully merging this pull request may close these issues.
3 participants
@boginw @MaceWindu @linq2dbot