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.

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

Fix conflicting annotations for multiple-database scenario #892

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

afn
Copy link

@afn afn commented Jul 29, 2021

Given a Rails application with multiple databases, tables in two databases with the same table name, and corresponding models with different names, some files get annotated with the wrong table's schema.

The issue is that some of the patterns include %TABLE_NAME%, which will be identical between the two tables, even though they have different model names.

This fixes the issue by eliminating the use of the table name when the model is not connected to the primary database.

Fixes #891.

Copy link

@pkopac pkopac left a comment

Choose a reason for hiding this comment

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

Our app also got affected, I had to temporarily disable other databases to be able to annotate correctly.

Given a Rails application with multiple databases, tables in two
databases with the same table name, and corresponding models with
different names, some files get annotated with the wrong table's schema.

The issue is that some of the patterns include `%TABLE_NAME%`, which
will be identical between the two tables, even though they have
different model names.

This fixes the issue by eliminating the use of the table name when the
model is not connected to the primary database.

Fixes ctran#891.
@afn afn force-pushed the fix-891-multiple-database-table-name-conflict branch from 845978f to 3ab4ab5 Compare July 2, 2024 15:10
@afn
Copy link
Author

afn commented Jul 2, 2024

I've just rebased this branch to develop. Note that this branch previously had an integration test for the new behavior, but since the integration tests were removed in #959, these tests are no longer included.

If integration tests are reintroduced upstream, they're in this commit: 385c293.

@drwl
Copy link
Collaborator

drwl commented Jul 8, 2024

@afn sorry to hijack the thread a bit, but if you're interested I could get this added to my fork https://github.com/drwl/annotaterb. Since I'm still on the list of maintainers for this gem I get pinged about activity by Github every now and then.

I would just need instructions on setting up a proof of concept Rails app with the case you're talking about (multiple databases + models with same table name).

@afn
Copy link
Author

afn commented Jul 8, 2024

Thanks @drwl!

I've pushed an example app to https://github.com/cygnuseducation/annotate-test-app. This test case doesn't actually end up incorrectly annotating the files, but it does incorrectly detect that there are relevant changes to the annotations:

➜  test-app git:(main) annotaterb models
Annotating models
Annotated (2): test/factories/users.rb, test/factories/users.rb

See also the integration test in 385c293.

Thanks!

@drwl
Copy link
Collaborator

drwl commented Jul 8, 2024

@afn would you mind if we continued discussions in drwl/annotaterb#135?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect annotation of factory files when different models share the same table name in different databases
3 participants