Critics and their API
Thomas O'Brien
Recently I've been developing a new feature for SQL Power Architect,
the critics, and we decided it would be best to outline the API and
some of our design decisions and where we want to take critics.
Before getting too deep it is helpful to understand what critics are
and what they are useful for. The critics system in SQL Power
Architect is meant to be equivalent to a spell checker for a word
document. Over the process of developing or maintaining a data model
anyone might make changes that cause an error when forward
engineering. With critics enabled these errors become much more
visible.
One example that exists in the latest builds on our continuous
integration server is when columns get mapped incorrectly on a
relationship. If you have two tables connected with a relationship and
you change the data type of a column in one table but not the other
criticizing the data model will display an error in the criticisms
list at the bottom of the play pen and will place a red X beside the
relationship line in the play pen. Further, right clicking on the X in
the play pen, and soon on the table, allows you to choose one of two
ways to fix the error without having to actually go to the columns of
the tables.
At current anyone can write their own critic and compile the critic
into SQL Power Architect or commit or submit a patch. In the future we
hope to allow users to create new critics directly in SQL Power
Architect.
At the end of this post is a short description of what you need to do
to create your own critic.
There are three classes/interfaces that make up the basis of critics.
These classes are Critic, Criticism, and QuickFix. With these classes
are the Criticizer which makes decisions on how to criticize a model.
To manage the critics and their settings is the collection of classes
that include the CriticManager, CriticGrouping, and the abstract
CriticAndSettings class.
The Critic interface is a simple interface with only one main method,
the criticize method. Any class implementing this interface must
implement this method to decide if there are problems with the single
object passed to the method.
-Currently each critic should only criticize the object passed to it
and not move to parents or children of the object being criticized as
it is the job of the Criticizer to decide how the objects should be
traversed. While the restriction that only the object passed to the
critic is a large restriction it makes criticizing an object model
simpler while the object model is being changed by a different thread.
In the near future we hope to add in a less restrictive critic
interface that will let the critic have access to siblings, children,
or its parent.
-Another aspect important to critics is they should not change the
object model that is being criticized. This includes the act of
populating objects, a critic should not cause any children of an
object to be populated.
-To report problems with an object a critic returns a list of
Criticisms. Currently a critic is allowed to make multiple criticisms
on an object, each with any number of quick fixes.
The Criticism class is an immutable class that describes what is wrong
with an object. The description field is important to a criticism as
it is the way the problem with the subject is given to the user. The
description field should normally contain information specific to the
subject to help the user understand what is wrong.
When creating a Criticism any number of QuickFix objects can be given
to it. A quick fix is not necessary for every criticism and some
criticisms may want several options. These quick fix objects give
users a one click solution to the problem raised by the criticism. The
description in a quick fix needs to give the user a strong
understanding of what will occur if they choose to use the quick fix
so it is important to give a helpful description, normally with an
example using the properties of the object that will be changed. Each
quick fix also has the ability to apply its changed as described to
the subject object.
To do the actual criticizing of a model the Criticizer class is used.
Currently there is only one implementation for doing criticisms in the
Criticizer which is for walking through the objects in the play pen
and passing the objects to the critics. In the future we may have more
implementations of the Criticizers which would allow for different
ways of walking through the same set of objects or for walking through
different types of object models.
Now that the basics of the critics classes have been outlined the
remaining classes involve the settings of the critics and their
implementation.
The CriticAndSettings abstract class is the class that implements both
the Critic interface and contains the implementation for all of the
basic settings of a critic. All of the current critics extend this
class so they only need to implement the Critic's criticize method.
The other important part of this class is to provide a thread safe
method of accessing a critic's settings in the future. We may want to
run the criticizing of parts of Architect on a background thread so
simplifying where the properties of the critics belong will make
changes like this simpler.
-Currently there are only two settings that a critic can have, a
severity and a platform type. The severity of a critic decides how the
critic will appear in places like the data source tree or the play pen
or if it will appear at all. The platform type works as a form of
grouping and can be set to any string in the future but there is an
enumeration of strings being currently used called
StarterPlatformTypes. In the future we plan to let users be able to
create critics from a UI in SQL Power Architect directly at which time
they will be able to place their critic in an existing or new platform
type. The platform type value also allows for critics to be enabled or
disabled as a group through the CriticGrouping class.
-Currently there is no way to set parameters for a critic but we would
like to change that after the first iteration. Allowing for critics to
be configured at runtime would allow users to specify required minimum
and maximum name lengths or preferred naming conventions and more. At
current if you want to make a critic that makes criticisms if a name
is too short you would have to create a new critic with the set
minimum length defined as a static value and re-compile the critic
into SQL Power Architect.
How to create a critic:
Creating a critic is quite simple in SQL Power Architect.
1) Create a new class in ca.sqlpower.architect.ddl.critic.impl that
extends CriticAndSettings.
2) In your constructor make a call to the super constructor giving a
platform name to group the critic by (see StarterPlatformTypes) and a
name that describes what will be criticized.
3) Implement the criticize method. For most critics you will want to
return early for most object types. The critic can create any number
of criticisms on the object but avoid exploring too far away from the
object you are given in terms of parent and child objects.
4) Add your critic to the STARTING_CRITICS list in the CriticManager.
At this point your critic will be in SQL Power Architect. If you start
the app and open the critic manager under the window menu you should
see your critic available. Next you can try out your critic by setting
up an invalid play pen and pressing the criticize button in the top
tool bar.
Feel free to check out the current implementation, respond with any
suggestions on how to improve the current design, or samples of your
own critics.
Thomas
Thomas Kellerer
I like the new implementation, especially the fact that I can select which implementations I want to check (those settings should be saved in the project though). And the error badge at the columns is quite nice.
However some things are not yet working as expected ;)
I just ran it on a very simple model, and I the following entry:
"ERROR","birthday: DATE","Generic","Physical name not legal for"
I don't understand why the name is marked as an error. "birthday" should match the Regex defined in AlphaNumericNameCritic as far as I can tell.
And the message is not very nice either because it's not a complete sentence.
If a Generic critic is enabled, as well as the DBMS specific version (e.g. AlphaNumericNameCritic and OraclePhysicalNameCritic I think the Generic one should be disabled (or ignored). I guess the easiest solution would be to allow selecting individual critics in the Critic Manager window. The more sophisticated solution would be to check the inheritance tree, and disable all ancestors of an enabled critic.
The error messages should ideally go into a ResourceBundle.
I don't think users that chose to run PA in their native language would like to see english error messages ;)
If you want, I can take care of that (English and German)
And finally there is an error that I don't understand at all:
"ERROR","film_id - film_title.film_id","Generic","Physical name not legal for Column Mapping"
What does this tell me? I tried to find the place where this is generated, but couldn't spot it.
Cheers
Thomas
Thomas O'Brien
At current the critics are still being cleaned up so some of the
wording and design still needs to be corrected. That being said I very
much appreciate trying them out to find problems and any help in
correcting problems! I will try to answer all of your questions or
comments.
> "ERROR","birthday: DATE","Generic","Physical name not legal for"
>
> I don't understand why the name is marked as an error. "birthday" should match the Regex defined in AlphaNumericNameCritic as far as I can tell.
> And the message is not very nice either because it's not a complete sentence.
name set to the empty string. It would make much more sense to have
the physical name match the logical name unless a user changes it.
This sentence ends up becoming unreadable when the physical name is
empty because it is trying to say "Physical name is not legal for the
column with the physical name (empty string)". It would read better if
it was changed to something like "Physical name is not legal for the
column with the logical name (birthday)." or something similar.
As a side note, to help find the critics that an error is about you
can click on the row in the table and it will select the object the
critic is on in the play pen.
>
> If a Generic critic is enabled, as well as the DBMS specific version (e.g. AlphaNumericNameCritic and OraclePhysicalNameCritic I think the Generic one should be disabled (or ignored). I guess the easiest solution would be to allow selecting individual critics in the Critic Manager window. The more sophisticated solution would be to check the inheritance tree, and disable all ancestors of an enabled critic.
its error type to "Ignore". For this specific case you can set the
generic critic to be ignored and the DBMS one to be something else.
One limitation to the inheritance tree idea would be if you have two
critics that criticized different aspects of an object but extended
the same class. For example, if you had a generic critic that forced
all names to be only alpha-numeric and a platform specific critic that
limited the name length but not the characters you would want to have
both critics on at the same time. Maybe we can introduce a type of
hierarchy in a different way in the future.
>
> The error messages should ideally go into a ResourceBundle.
> I don't think users that chose to run PA in their native language would like to see english error messages ;)
> If you want, I can take care of that (English and German)
was being a bit relaxed with this aspect until I had the system in a
better state. When I get back onto Architect next week I can start on
correcting the text messages and pull them into a resource bundle. I
want to rewrite most of the error messages at current but if you want
to take that feel free to.
>
> And finally there is an error that I don't understand at all:
>
> "ERROR","film_id - film_title.film_id","Generic","Physical name not legal for Column Mapping"
>
> What does this tell me? I tried to find the place where this is generated, but couldn't spot it.
illegal names. In the SQLRelationship class you will see the class has
children of type ColumnMapping. A column mapping is an object that
points to two columns, one in the pk table and one in the fk table, of
a SQLRelationship. The error says that the physical name is not legal
but it should not matter because it doesn't actually get used in a
database.
Thanks for the comments and the feedback!
Thomas
Thomas Kellerer
> There is an existing bug that creating new columns has the physical
> name set to the empty string. It would make much more sense to have
> the physical name match the logical name unless a user changes it.
> This sentence ends up becoming unreadable when the physical name is
> empty because it is trying to say "Physical name is not legal for the
> column with the physical name (empty string)". It would read better if
> it was changed to something like "Physical name is not legal for the
> column with the logical name (birthday)." or something similar.
Yes indeed that was the problem. After adding a physical name, the error went away.
I changed PhysicalNameCritic to detect that situation and return a proper error message.
Even if the bug for new columns was fixed, the user could still accidently delete the physical name, so I think it makes sense to show that in the critic.
> As a side note, to help find the critics that an error is about you
> can click on the row in the table and it will select the object the
> critic is on in the play pen.
Yes, I noticed that. Very nice!
> At current you can disable a critic in the critic manager by changing
> its error type to "Ignore". For this specific case you can set the
> generic critic to be ignored and the DBMS one to be something else.
Ah, I didn't checkt the dropdown, sorry.
> One limitation to the inheritance tree idea would be if you have two
> critics that criticized different aspects of an object but extended
> the same class. For example, if you had a generic critic that forced
> all names to be only alpha-numeric and a platform specific critic that
> limited the name length but not the characters you would want to have
> both critics on at the same time. Maybe we can introduce a type of
> hierarchy in a different way in the future.
Yes, good point.
> I think this is a mistake on what the critics are identifying as
> illegal names. In the SQLRelationship class you will see the class has
> children of type ColumnMapping. A column mapping is an object that
> points to two columns, one in the pk table and one in the fk table, of
> a SQLRelationship. The error says that the physical name is not legal
> but it should not matter because it doesn't actually get used in a
> database.
So wouldn't it make sense to skip ColumnMapping objects in the PhysicalNameCritic
Regards
Thomas