ASSERTION in openehr_expression_104.bmm is not in accordance with the specifications

op_table is static, so it acts like a singleton object. The owning class just populates the table at startup time. You could also use the GoF Singleton pattern to achieve the same effect (which doesn’t use inheritance). In most languages these days you don’t need to inherit, you can just reference BUILTIN_OPERATORS.op_table(or in Java - BuiltinOperators.opTable().#

It’s not an enumeration though, it’s a table of the form:

|"plus"       | "+ "
|"multiply"   | "*", "."
etc

Any logical operator might have a number of alternative symbols.

I sometimes forget how inexperienced I must sound with such questions :flushed:

We both know that BUILTIN_OPERATORS is a singleton just by looking at the model.
However I’m asking for a friend - my generator :wink:

The generator knows only what is in the computable specifications - the BMM files.

I posted the question because there is no information that BUILTIN_OPERATORS is a singleton or that op_table is static in the BMM.

BUILTIN_OPERATORS is the second ancestor of the OPERATOR_DEF_BUILTIN.
OPERATOR_DEF_BUILTIN “extends” OPEREATOR_DEF and “implements” BUILTIN_OPERATORS.

Since the BUILTIN_OPERATORS.op_table is not marked as static it needs to be “implemented” in OPERATOR_DEF_BUILTIN. The only way I know how to “implement” a property from an ancestor is by adding it to the OPERATOR_DEF_BUILTIN class (I tried with mixins but that didn’t work out).

BUILTIN_OPERATORS.op_table is marked as mandatory in BMM which means it must be present for every OPERATOR_DEF_BUILTIN instance.

A solution would be to add “is_im_singleton” and/or “is_im_static” to the BMM classes/attributes. Then BMMs would have the same information as we. I’m not sure if that is a realistic expectation?

So I have two options with the current BMM:

  • remove BUILTIN_OPERATORS as the second ancestor of OPERATOR_DEF_BUILTIN
  • remove “is_mandatory” for BUILTIN_OPERATORS.op_table so that the “fromJson()” doesn’t report an error if it is missing in OPT2 JSON.

I’m aware of only one other class that has to “implement” properties found in its ancestors. It is where AUTHORED_ARCHETYPE “extends” ARCHETYPE and “implements” AUTHORED_RESOURCE which has mandatory properties.
The generator can handle this scenario even for reading the OPERATIONAL_TEMPLATE from JSON.

Edit:
The problem is setting mandatory properties at the instantiation of OPERATIONAL_TEMPLATE instance:

  • OPERATIONAL_TEMPLATE constructor can call its ancestor’s constructor in AUTHORED_ARCHETYPE.

  • AUTHORED_ARCHETYPE constructor can call its ancestor’s constructor in ARCHETYPE.

  • AUTHORED_ARCHETYPE constructor cannot call its second ancestor’s constructor in AUTHORED_RESOURCE.

  • Thus AUTHORED_RESOURCE’s mandatory properties cannot be set when creating an instance of OPERATIONAL_TEMPLATE.

Yes, it should be. In Archie this is configurable because of when I built it, it was not yet standardised as _type, I must have used the wrong configuration in the vs code extension. Will create an issue and fix later

Is this just for the archetype slots? Then it can be simplified and all archetypes I have seen will just parse.

If it is for the rules section, then you need more than just a binary operator.

By the way, it looks like I can make Archie and the VSCode plugin easily output JSON as in the model in that diagram generated from the BMM for archetypes and OPT 2s, configurable to switch to the new version. Just the operator def might be different, as that is an enum in archie. Even the new model looks mostly possible, but will be more work to do.

i will try soon if it’s possible. It will still all just parse to the same model in Archie that I will not change right now.

Yes.


Since my OPERATIONAL_TEMPLATE fromJson() method is generated from AM BMM the OPT2 JSON must be in exactly the structure of the AM specifications.

I didn’t know if you will have time to add the required changes to Archie so I wrote OPT2 to JSON converter.

There appear to be more differences, unfortunately.

Differences between the model in Archie and Expression Language (EL) that are not simple renames that I found so far:

  • EXPR_FUNCTION.function_def is of type FUNCTION_DEF. It is EXPR_FUNCTION.function_name as a string in Archie. There is no function_def class in Archie.
  • OPERATOR_DEF_BUILTIN is an enum in Archie (renames are easy, parsing/serialization slightly more tricky)
  • EXPR_TYPE_DEF is the enum EXPRESSION_TYPE in archie (renames are easy, parsing/serialization slightly more tricky)
  • there is no ASSINGMENT in Archie, just variable declaration, not even in the grammar.
  • the for all statement is missing in the specification. It cannot be expressed as a simple binary operator, and in archie is a separate class inheriting from operator with variable_name, left_operator (the expression pointing to a collection of values) and right_operand (the expression to be evaluated as an assertion for every member of the collection)
  • precedence_overridden is in binary operator in the spec, in expression in Archie. Minor, in archie it’s possible to store more places where () can appear in the grammar, but they are pretty much meaningless in execution if not around an instance of OPERATOR

I might find more later.

2 Likes

Right, now I get where you are coming from. Sorry for the basic info (but quite often we have newbies visiting these posts as well, so us experienced folk should not hold back on simple explanations).

All the issues you point to in the BMM are addressed in the new form, i.e. the one that is not yet quite ready. I’ll have to have a think about what we can do in the short term…

Skimming your ideas - they sound ok to me - definitely you can remove the inheritance for the operator table, that’s not a modern way to access a singleton object. I’ll have to ponder the rest though for a bit longer.

1 Like

My conclusion is that this is a bit of a mess. Archie has an exact implementation as the model was specified when I implemented it. The only deviations in the serialisation are the missing EXPR_ prefixes to the type names, and missing op_ prefixes for the operator enum, and the addition of a modulo operator that was missing from the class definition. This matches exactly with the serialisation in the AOM 1.4 specification, at least for the archetype slots.

For at least the archetype slots, I can add a configurable rename so it will output the literals as EXPR_LITERAL (they were called LEAF before!) or the old version. The operator_def is tricky. If I have to make a sort of possibility of a migration path, I can output and accept both operator (enum, so a string), and operator_def (object with identifier). The operator_def seems very strange to me in a serialisation format. I think the op_table does not belong in the json as it has no meaning there, and certainly cannot be changed from outside a system.

Then if we decide to switch to the BMM_ASSERTION, everything will change again, the operator def will change to a rather complicated function_def. That will be a much harder migration path, probably with some way to accept and generate both versions of the formats. note that this is not just for the rules section, also the archetype slots, although that is just a subset of the entire language.

First attempts gives the following json for an expression in an archetype slot. This is a bit different than @NeoEHR s example - this one has the following differences, which I consider to follow the spec:

  • EXPR_ARCHETYPE_REF for the path reference
  • right operand is a EXPR_CONSTRAINT instead of a literal.

And where it still deviates from the standard, but I think the standard needs a correction:

  • EXPR_ARCHETYPE_REF has no ‘item’ field, since it does not make sense to add it (why and what should it be filled with?)
  • no op_table, since it makes no sense to include at all.
  • both operator, operator_def and symbol for reasons stated above
  • some convenience fields such as path added, that are very useful in javascript. I could easily add an option to get rid of them, as I have done in the RM.
 "expression" : {
            "_type" : "EXPR_BINARY_OPERATOR",
            "type" : "BOOLEAN",
            "precedence_overridden" : false,
            "operator" : "op_matches",
            "symbol" : "matches",
            "left_operand" : {
              "_type" : "EXPR_ARCHETYPE_REF",
              "precedence_overridden" : false,
              "path" : "archetype_id/value"
            },
            "operator_def" : {
              "identifier" : "matches"
            },
            "right_operand" : {
              "_type" : "EXPR_CONSTRAINT",
              "precedence_overridden" : false,
              "item" : {
                "_type" : "C_STRING",
                "rm_type_name" : "string",
                "node_id" : "id9999",
                "constraint" : [ "/openEHR-EHR-INSTRUCTION\\.medication\\.v1/" ],
                "attributes" : [ ],
                "logical_path" : "/",
                "allowed" : true,
                "required" : false,
                "prohibited" : false,
                "path" : "/"
              }
            

If that is true, it must be that I am working off the wrong version (there is no obvious version to work off, because there was no release). But since I have created the BASE 1.0.4 branch to capture the intended version, we’ll keep mutating that until it matches the implementations.

Hm I must admit I don’t even remember that, must have been a very old version - I have had a habit of using the ‘expr_’ prefix for over 20y!

I think we should mutate the branch version of the spec, not your implementation. Let’s not create more work.

Correct, that is like including a constant in a data serialisation - not useful and not part of the data.

We won’t do that in this branch version. The new Expression Language will do that, but that’s a completely different thing.

I’ll start having a look at some of the things you have listed. But remember - whatever version you originally worked off, it was incomplete, and contained errors, so what we specify now will necessarily be some upgrade on that original.

Sorry, the expr_ prefix was in the zpecification, that omission was my mistake - I mean the original literal was called EXPR_LEAF. This means I have to do some renames anyway, which is annoying but fine.

I have time to respond to the rest tomorrow.

I wanted to use these two in my JSON this morning and found out they are missing in the BMM files (that is probably why I used EXPR_LITERAL - I have to learn to always check the web specifications and compare them with the BMMs). They are in the specifications for AOM 2.0.6 and AOM 2.2.0.

@thomas.beale Can they be added please? Pieter also made a comment about the EXPR_ARCHETYPE_REF.item field:


BUILTIN_OPERATORS in OPERATOR_DEF_BUILTIN:

@thomas.beale Can the BUILTIN_OPERATORS as the second ancestor of OPERATOR_DEF_BUILTIN be removed from the BMMs?

The expr_ prefix was definitely there, I for some reason did not implement it. Looks like I implemented specifications-AM/docs/AOM2/AOM2.html at 5addcf864d9ab0f1439cf86ac12311c4fbf7b72b · openEHR/specifications-AM · GitHub .

Modification I made that I do not know the reason for:

  • instead of a LIST<RULE_STATEMENT>, I added a RULES_SECTION class, with LIST<RULE_STATEEMNT> and a string content. Not sure why.

Modifications I had to make to make this work:

  • added a FOR_ALL_STATEMENT class, because it does not fit in BINARY_OPERATOR (missing the variable declaration), see archie/aom/src/main/java/com/nedap/archie/rules/ForAllStatement.java at master · openEHR/archie · GitHub
  • ASSERTION does not have a variables attribute. Does not make sense to have, and the class wasn’t defined in the spec back then.
  • EXPR_ARCHETYPE_ID_CONSTRAINT is implemented, but never used, since at grammar level, this cannot be easily instantiated - a normal Constraint with a C_STRING is used in these places.
  • MODEL_REF has a variableReferencePrefix attribute in Archie, to indicate the path starts with a variable (as used in for_all)
  • OPERATOR_KIND has op_module added
  • added FUNCTION to implement the function calls specified in the language spec, which was missing in the object model, with attributes:
String functionName;
 List<EXPR_ITEM> arguments;

So, what I can relatively easily change:

  • renaming of classes, probably even parsing both and generating only the new ones, configurable to not break older clients
  • adding new attributes, like operator_def, next to the existing one
  • making the ARCHETYPE.rules attribute return a LIST<RULE_STATEMENT> again, configurable to support older clients.
  • changing inheritance of FOR_ALL_STATEMENT to binary operator instead of operator.

And one for readability:

  • OPERATOR_KIND is an enum, but not an integer - it uses the string name, so op_eq, etc. Well actually in archie for some reason, just ‘eq’, but that can be changed.

So, what should we change, and in archie or the specification?

2 Likes

I’ll need a few days to get onto this, but let’s change that BASE 1.0.4 spec as much as coherently possible - if it is to be useful, it might as well be reverse-engineered from your Nedap implem - which we know works. If we get stuck (probably we will on some detail), we’ll work out a solution.

I am not in favour of you having to do more work in your implem - the reason for this branch EL spec is to provide a spec that your implem is based on. So let’s get as far as we can before you start making any Archie changes.

Pull request to do the rules changes in Fix json rules by pieterbos · Pull Request #377 · openEHR/archie · GitHub . Mostly configurable to be able to upgrade archie without too much trouble, and parsing is backwards compatible for easier upgrading.

Most work there is the configuration and the mechanism, not the renaming. I made it a draft pull request. So, @thomas.beale , are the names in Fix json rules by pieterbos · Pull Request #377 · openEHR/archie · GitHub as they should be?

I will add tests of course. After this is done, next week we hope to merge all this and be able to release a major archie release soon. Since it’s not fully compatible with Archie 1, will likely be 2.0.0 (semver).

The changes look right to me - I wonder if you are making things too configurable however? The Archie code will break for sure but I suspect your (i.e. Nedap) system is the only user of that code so far - EhrBase doesn’t have any rule logic in its templates as far as I am aware - so dealing with the breakages locally is pretty trivial.

You could arguably consider this change a patch-level change to Archie if we agreed consider the missing name prefixes an ‘error’ - not casting aspersions within this group, I’m just thinking about how it looks from the outside / under semver logic.

There are more changes that break things, also to the API of Archie, so it’s a major release.

No, not too configurable. We have android apps parsing these rules with Archie, and we need to be able to update both our server and our apps without waiting for all apps to be updated. This means we need a proper migration path where this configurability is absolutely necessary.

1 Like

Good point - what is in the JSON matters, and if you don’t want to be chained to doing some sort of magic whole-of-system instant update, then you have to allow both.

I do wonder though if the config settings can be dispensed with in a later ?minor version - to reduce complexity a bit, and also not to give new Archie users the idea that there really is an intentional choice available here?

Yes, some of these config settings (not all!) can be removed in a later version. I could mark the configuration settings as deprecated right from the start. But actually removing them will be a major version no matter what, even if they are deprecated, since it changes the behaviour considerably and will break apps if it is still used.

1 Like

Great update Pieter!


@thomas.beale

  • EXPR_CONSTANT
  • EXPR_CONSTRAINT
  • EXPR_ARCHETYPE_ID_CONSTRAINT
  • EXPR_ARCHETYPE_REF

are missing in “openEHR_am_206.bmm” (and 210, 220, 230).
AOM 2.0.6