fm_IDL.xml does not indicate all required fields

[Jason Stephenson]

Evergreen versions: all

The IDL can accept an attribute on fields to indicate that they are required, "oils_obj:required='true'". Many (most?) fields are required, but do not have this attribute in the IDL. A field is required if the database schema indicates a "not null" constraint on the column.

The IDL and schema should be audited and the oils_obj:required='true' attribute added to all required IDL entries. This would benefit us in a couple of ways.

The Angular fm-editor components (also used by the Admin interfaces) will indicate required fields when they are marked as such in the IDL, and will prevent an attempt to save or update an entry when required fields are missing values. By updating the IDL, these interfaces will be more accurate and staff will encounter fewer unexplained instances of failures to save data.

Clients could take advantage of the knowledge of required fields when creating new objects or checking modified objects for validity. This could lead to decreased development and debugging time and more robust applications.

[Jason Stephenson (jstephenson)]

The working branch user/dyrcona/lp2050227-update-idl resolves this issue by adding the oils_obj:required attribute to required fields. "Required" is defined as a real field that exists in the database that can be updated (i.e. the object is not readonly and the field or object are not virtual), that has a "not null" constraint and lacks a default value.

The branch has 4 commits:

1. Adds only the required attribute.
2. Regularizes spacing in fields based on libxml2 writing out the IDL, and adjusting indentation based on the vim and Emacs settings in the file. (NB: There may be some whitespace "errors" that were missed.)
3. Fixes a typo and removes a duplicate permissions entry based on validating the result of the above with the fm_IDL.xsd schema.
4. Adds a release note to the "Architecture" section. (I struggled with putting it in Architecture or API. If you commit this and want to move the release note, I will NOT be fussed.)

URL for the branch is: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp2050227-update-idl

I have lightly tested this by running the Perl tests and adding a few settings through the Angular admin interfaces. Everything that I've tried works.

[Jason Stephenson (jstephenson)]

The branch was rebased on main as of the time of this comment.

[Galen Charlton (gmc)]

I think this is quite close, but I have a question about commit 84fa09ce0c0. This isn't purely a whitespace-and-reformatting patch as described in the commit message, but also includes some substantive changes.

Some of them make sense: e.g., the patch series starts by setting vandelay.queued_bib_record.create_time as required, but 84fa09ce0c0 reverses that. By your criteria, it should _not_ be marked required, as create_time has a default value.

Others do not: 84fa09ce0c0 reverses the changes made by the first patch in the series and marks several fields in metabib.display_entry as not required. However, the first patch was correct - as non-NULL fields with no default value, they should have been marked required.

What's going on with that commit?

[Jason Stephenson (jstephenson)]

Galen, any non-whitespace changes in 84fa09ce0c0 were mistakes that I did not notice. I'll take this back and fix it.

I messed with this quite a bit over a few days and rebased it on main a couple of times. One of those rebases likely had some unintended side effects.

[Jason Stephenson (jstephenson)]

I had a look at this again this morning, and I actually don't think the removal of the required fields on those IDL entries is wrong. When you review my criteria for a field being required, then these fields are not required:

Required fields are that those that come from a database table, have a "NOT NULL" constraint in the schema, and do not have a default value assigned in the database.

"[C]ome from a database table," means literally that. Readonly entries based on views don't need to have readonly set since they cannot be updated.

Many of the examples that changed are from readonly IDL entries, or the database has a default value.

All of that being said, my process skipped fields that already had the readonly attribute set and skipped readonly IDL entries. If they had these attributes before, they should not have changed. It also bothers me that these fields changed during the whitespace clean up. I am not sure what happened there. As I mentioned previously I worked on this over several days, I used a couple of branches and had to rebase it a couple of times as other commits made changes to the IDL. I also did a few of these steps more than once.

I am not happy with how things have turned out, so I'll start this over in another branch.

[Galen Charlton (gmc)]

This comment is just me thinking things through aloud.

Let's take metabib.display_entry. Its source, field, and value columns in the database are NOT NULL with no default value, so those columns ordinarily should be required -- if one were meant to edit rows in that table directly. In main, those are _not_ marked as required in the IDL.

But that class in the IDL is marked as readonly - not because it's a view, but because its rows are maintained solely by other database triggers.

Consequently, neither CStore nor PCRUD will create methods to change data in that class, and thus, whether or not individual fields are required doesn't matter.

[Galen Charlton (gmc)]

So upshot, I now better understand what's going on and will await your revised branch.

[Jason Stephenson (jstephenson)]

I have a pushed an "new and improved" branch to working/user/dyrcona/lp2050227-update-idl-take2 (https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp2050227-update-idl-take2).

This one should hot have the problem of removing the oils_obj:required entries from IDL entries, even if they are not required.

I think it has one more commit than the previous branch because I split the indentation clean up from the changes made by the libxml2 program used to add the required attribute to the required fields. (I can share that program if anyone wants to see it.)

I also added a few more sentences to the release notes about what was done.

[Galen Charlton (gmc)]

Thanks, I shall take a look. I wouldn't mind a peek at the libxml2 program.

[Jason Stephenson (jstephenson)]

OK, Galen! I've attached it here.

[Mike Rylander (mrylander)]

I'm sorry I haven't noticed this work until now, but I want to raise a caution flag before a commit happens and it becomes harder to revise any decisions made in this branch.

Each XML namespace used in the IDL is meant to signal information to different layers of the software (or, different audiences). I'm concerned that we're crossing purposes a little and just want to make sure we consider the background and context of what exists now.

For the issue at hand here, I think there are two namespaces that matter: oils_obj and oils_persist.

oils_persist is meant to describe the database layer, and signal to logic that touches the database directly about things that are relevant to that logic regarding the fields/columns, but that are not easily discoverable through inspection of the DB object structures or DDL, but are necessary for properly constructing queries using those columns/fields.

oils_obj is meant to describe the in-memory objects built from table rows (in most cases), and how both code and users of the code should expect to interact with the fields/properties.

ISTM, if we want to describe the nullability of a column, we should invent a new attribute in the oils_persist namespace. If we're just trying to identify fields that a user must supply a value for, we should (as this branch does) make fuller use of the oils_obj:required attribute. However, I don't think they're the same thing, even if they significantly overlap, because as Galen says in comment #6 above there are cases where "required" isn't necessary or even meaningful.

For a full list of the "defined" attributes, such as they are, you can look at fm_IDL.xsd, which sits right next to fm_IDL.xml.

Thanks, Jason and Galen!

[Galen Charlton (gmc)]

I think that the degree of overlap between null-ability and whether a field should be considered required in (e.g.) the FM editors is going to be close enough for what I understand is the primary purchase of this patch set.

However, I will be on the watch for cases where an empty string might be appropriate as a value for a not-NULL field from the application's point of view.

[Jason Stephenson (jstephenson)]

The goal of this bug/branch is to tell users what fields they have to fill in. Checking the nullability of database columns seemed the way to do that.

[Galen Charlton (gmc)]

I've compared the output of fix_idl_required.pl with the results of the patchset; end result is as expected.

[Mike Rylander (mrylander)]

I withdraw my caution flag, since the goal is user-level and that matches oils_obj. Thanks for entertaining my diversion! :)

[Galen Charlton (gmc)]

Signoff branch pushed to working/user/gmcharlt/lp2050227_signoff.

There are at least three columns now marked required where the empty string can be valid value:

asset.call_number.label
asset.call_number_prefix.label
asset.call_number_suffix.label

The first (call number labels) is taken care of via the holdings editor changes in bug 1821950.

The second and third (call number affixes) are a special case. The ID = -1 rows must have the empty string as their value, but I can't imagine at present needing rows where the empty string call number affix value must also exist at other owners in the OU hierarchy.

As a consequence of this patch, the -1 affixes can be opened in the Angular admin interfaces but not saved. However, since the only time you'd need to do this is manually correcting the default entries if the root node of your OU tree does not have ID 1, I think this is enough of an edge case to ignore for the purpose of this patch (though it should be written up as a new bug).

[Galen Charlton (gmc)]

In my opinion, this should be backported to at least 3.12. Backporting to 3.11 would be a good idea as well, although presumably doing so will require a bit more effort.

[Galen Charlton (gmc)]

Also, Jason, would you have any objection if fix_idl_required.pl were added to the tree (say in Open-ILS/src/extras, though it may also be a good idea to start a directory that's just for developer utilities). That way, it could become the basis of tests to catch any future cases where a new column should be marked as required.

[Jason Stephenson (jstephenson)]

Galen,

I'll respond to your latest 3 comments in order:

Regarding the asset.call_number columns being required and that potentially causing issues, I'm OK with removing the required attribute from these fields as long as doing so solves a problem, i.e. data being saved or not. (Perhaps prefix and suffix should have default values of -1? That's a different bug if so.) If you want to leave this as is, I'm OK with that, too.

I didn't look specifically at asset call numbers with these changes. I was mainly focused on the settings editor and related interfaces. I can also go back and test the volume editor specifically if you want.

I'm OK with backporting to 3.12 and 3.11. I didn't initially target this bug at those releases because I was concerned about drift in the IDL between those branches and main. Presently, I don't think there is any drift between 3.12 and main, and I have not checked 3.11.

Yes, if you want to add the fix_idl_require.pl program to examples or some new delvelopment support scripts directory, that would be fine with me. Perhaps it could be expanded in the future to check more apsects of the IDL?

[Galen Charlton (gmc)]

I think that asset.call_number_{prefix,suffix}.label can stay required, and I'm satisfied that the holdings editor will do the right thing.

Regarding IDL drift, I confirm that there currently isn't any between main and rel_3_12. There are some differences between rel_3_12 and rel_3_11, but not enough to make a backport painful IMO.

I'll start a separate bug for putting fix_idl_require.pl somewhere in tree; I don't think it needs to hold up merging the IDL changes themselves.

[Jane Sandberg (sandbergja)]

Thanks, Jason and Galen! Pushed to main.

[Galen Charlton (gmc)]

Thank you, Jane.

Noting the discussion about backporting in this bug, did you have a specific reason for not backporting at least to rel_3_12?

[Jane Sandberg (sandbergja)]

Thanks for pointing out the backporting discussion, Galen, I saw that and then completely forgot about it. I pushed this to rel_3_12 accordingly.

[Galen Charlton (gmc)]

Thanks! I just wanted to ensure that there wasn't some blocker.

I'd also like to see this in 3.11, but I'll prepare a separate branch for the backport.