Avoid storing lints in catalog

[bekoenig]

Description

Calling Linters#lint from java is confusing. In first expectation this method should returns all found lints. But this method has a side effect in which all lints are stored in the catalog root and each table. So the caller has to replicate the object key (private constant), get all lints from the nodes and reset the catalog to get the original state.

How to Reproduce

No response

Relevant log output

No response

SchemaCrawler Version

16.22.3

Java Version

21

Operating System and Version

Win 10

Relational Database System and Version

DB2LUW 11.5

JDBC Driver and Version

current

[bekoenig]

I use this method in my new test extension (will be open sourced in the next few months) to check for new lints after each flyway migration. So I remove all known lints from the new result to get the difference. My current solution has some boilerplate and produces footprints at the catalog instance which I don't like as the catalog is also used for asserts with assertj-schemacrawler to check the schema changes after migration.

[sualeh]

@bekoenig How about if we nested the catalog and table lints as an attribute called {"schemacrawler.lint" = Map<String, Object>}. The inner map would contain the lints as usual. I could add a method to clear lints, which would simply remove the "schemacrawler.lint" attribute on the catalog and all tables. Will that work for you - a new method to clear lints?

[bekoenig]

Hi @sualeh,

thank you for your quick feedback! Your suggestion would work, but my idea is more api related.

My current solution looks like:

Connection connection = ...;
Catalog catalog = ...;
Linters linters = ...;

// void
linters.lint(catalog, connection);
// lints are state from linters
Collection<Lint> lints = linters.getCollector().getLints().stream().map(Lint.class::cast).toList();
// clear lints from catalog
catalog.setAttribute("schemacrawler.lint", new ArrayList<>());
// clear lints from tables
catalog.getTables().forEach(t -> t.setAttribute("schemacrawler.lint", new ArrayList<>()));

return lints;

My suggestion would be:

Connection connection = ...;
Catalog catalog = ...;
Linters linters = ...;

// lint returns collector
Collection<Lint> lints = linters.lint(catalog, connection)
    .getLints().stream().map(Lint.class::cast).toList();
// clear is not neccessary, because lints are not stored in catalog instance

return lints;

Greetings,
Ben

[sualeh]

Ok. I have started on a fix in the issue1750 branch.

[sualeh]

@bekoenig @JohReher - Please take a look at the code I just merged into main - 2bf6e86

It contains:

• Fix Avoid storing lints in catalog #1750 - avoid storing lints in catalog
• Run linters run in parallel
• Enhance lint report to allow streaming lints

You will be using the lint report rather than the lint collector, but you will have the same functionality.

I will release this as soon as I finish up the other work I want to get into the next release.

[sualeh]

@bekoenig @JohReher - Please use SchemaCrawler 16.23.1.

[bekoenig]

Hi @sualeh,

thank you very much for your improvements. You will get feedback in the next few days.

Greetings,
Ben

[bekoenig]

Hi @sualeh,

here is my promised feedback after refactoring from version 16.22.3 to 16.23.1:

I like your changes, but now there are no possibilities to filter known lints after linting process to create a report with new lints. LintReportGenerator needs an instance of LinterReport which boxes all lints. Constructor of LinterReport ist private and array with lints can't be mutated. My current workaround is to use the three parameter constructor of LinterReport by reflection.

Greetings,
Ben

P.S.: Some other change, which is not related to this issue is that getCrawlInfo in BaseLinter is removed. I used this method to implement generic rules which effects only specific database systems (like db2luw).

[sualeh]

@bekoenig Thanks for your feedback.
The idea is that consumers of the lint API only use LintReport. It deliberately mostly immutable - except for the title.

1. You can stream lints from LintReport using the stream() method. This is similar to what you were doing earlier with the lint collector and should work the same way. You were creating your own filtered array, and you can still do that.
2. You can get the crawl info from the LintReport as well.

If I am not understanding your use case, please provide some pseudo code like you did earlier. Or, please share your working copy of the assertj-schemacrawer library.

[sualeh]

P.S.: Some other change, which is not related to this issue is that getCrawlInfo in BaseLinter is removed. I used this method to implement generic rules which effects only specific database systems (like db2luw).

Meaning, are you writing a custom linter that needs the crawl info? If yes, then I can add it back in. Please let me know.

[bekoenig]

P.S.: Some other change, which is not related to this issue is that getCrawlInfo in BaseLinter is removed. I used this method to implement generic rules which effects only specific database systems (like db2luw).

Meaning, are you writing a custom linter that needs the crawl info? If yes, then I can add it back in. Please let me know.

Yes. By default all of our custom linters are enabled, but some linters are only needed for specific database systems.

[sualeh]

Got it. I will add that back in, or something similar.

[bekoenig]

@bekoenig Thanks for your feedback. The idea is that consumers of the lint API only use LintReport. It deliberately mostly immutable - except for the title.

1. You can stream lints from LintReport using the stream() method. This is similar to what you were doing earlier with the lint collector and should work the same way. You were creating your own filtered array, and you can still do that.
2. You can get the crawl info from the LintReport as well.

If I am not understanding your use case, please provide some pseudo code like you did earlier. Or, please share your working copy of the assertj-schemacrawer library.

// all lints before migration script
List<Lint> lastLints = ...;

// open connection from migrator
Connection connection = ...;

// generic config for our framework users
LinterConfigs linterConfigs = ...;

Linters linters = new Linters(linterConfigs, true);
Catalog catalog = loadCatalog(connection);

// linting process
linters.lint(catalog, connection);

// all lints after migration script 
List<Lint> currentLints = linters.getLintReport().getLints().stream().map(Lint.class::cast).toList();

// new lints after migration script
List<Lint> newLints = currentLints.stream().filter(l -> !lastLints.contains(l)).toList();

// here i want to create a report for newLints
new LintReportTextFormatter(...).generateLintReport( ??? );

This issue is not related to assertj-schemacrawler.

[sualeh]

@bekoenig I have introduced a lint report builder. This may be a little clumsy for you to use, but I have done this so that the lint report presents an immutable face, which I prefer. I will release a new version of SchemaCrawler soon.

[sualeh]

@bekoenig @JohReher - Please use SchemaCrawler v16.23.2. You will need to use LintReportBuilder to build your custom lint reports.

[bekoenig]

Works perfect. Thank you very much.