AmpersandTarski / Ampersand

Build database applications faster than anyone else, and keep your data pollution free as a bonus.
http://ampersandtarski.github.io/
GNU General Public License v3.0
40 stars 8 forks source link

XLSX-importer should NOT define relations except for data-analysis purposes. #1387

Open stefjoosten opened 1 year ago

stefjoosten commented 1 year ago

This issue started under a different title: Under specialization, UNI doesn't propagate to the interface functionality. That is useful to know when reading the comments.

What happened

I am making an application that has the following relation:

RELATION dienstniveau[BeheerEenheid*Dienstniveau] [UNI]

Then I use this relation in an interface:

INTERFACE Applicatie : I[Applicatie] BOX<FORM>
   [ Applicatie     : I                    CRud
   , dienstniveau   : dienstniveau         cRUd
   ]

By the way:

CLASSIFY Applicatie, Koppeling ISA BeheerEenheid

And here is what I saw in the user interface:

image

The interface behaves as if there is no restriction on the relation dienstniveau.

What I expected

I declared the relation dienstniveau to work on Beheereenheid and I expected that the CLASSIFY-statement implicitly declares the relation dienstniveau[Applicatie*Dienstniveau]. So far no problems. However, I also expected that the restriction UNI implicitly applies todienstniveau[Applicatie*Dienstniveau], because $A\preceq B\ \wedge\ (I{[A]};r{[B\times Z]}=r{[A\times Z]})\ \wedge\ uni(r{[B\times Z]})\ \Rightarrow\ uni(r_{[A\times Z]})$. For this reason, I also expected that the relation dienstniveau[Applicatie*Dienstniveau] inherits the built-in properties UNI. And by the same account I would expect it to inherit INJ, if I would have restricted dienstniveau[BeheerEenheid*Dienstniveau] to be injective.

Version of ampersand that was used

I used Ampersand-v4.7.1.

stefjoosten commented 1 year ago

Analysis

If my expectations are justified, adding

RELATION dienstniveau[Applicatie*Dienstniveau] [UNI]

should help. And indeed, this is what the interface shows:

image

As a consequence, I can work around the problem by adding the implicit declarations myself, like so:

RELATION dienstniveau[BeheerEenheid*Dienstniveau] [UNI]
RELATION dienstniveau[Applicatie*Dienstniveau] [UNI]
RELATION dienstniveau[Koppeling*Dienstniveau] [UNI]

In my code, I had to do this for 8 different relations, which is definitely not desirable.

hanjoosten commented 1 year ago

I would say the expectations are wrong. There is no such thing as inheritance in Ampersand, let alone inheritance of properties from one relation to another.

That said, there is no need for inheritance. In the the interface, there is a term. This term has the form Epsilon [Koppeling *BeheerEenheid]; dienstniveau[BeheerEenheid*Dienstniveau]. In the interface, we need to know iff that term is univalent. It should be, but this issue shows it isn't.

hanjoosten commented 1 year ago

Diagnosis

I created a small script:

CONTEXT Issue1387
   RELATION r[A * B][UNI]
   CLASSIFY C ISA A
   INTERFACE Test : I[C] BOX<FORM>
      [ C    : I    CRud
      , r     : r    cRUd
      ]
ENDCONTEXT

In the interface.json file we see what happens: image

This is correct. It proves that the error is at the side of the prototype frontend.

hanjoosten commented 1 year ago

@Michiel-s , could you investigate further?

Michiel-s commented 1 year ago

I'll have a look at it this weekend.

@stefjoosten Not a browser cache issue again? Just checking.. If you generate an application, and later add relation constraints and regenerate the application, you browser sometimes doesn't pick up the changes in the html files. Refresh browser with cache disabled might help.

But anyway I'll have a look. Maybe it's a bug.

Michiel-s commented 1 year ago

Analysis

I cannot reproduce this bug/issue.

Using: Prototype framework version: v1.16.0 (latest main version) Ampersand compiler version: v4.7.0 (included in prototype) same when manually injecting compiler v4.7.1 (mentioned in issue)

This is the script I ran:

CONTEXT "Issue1387"

    RELATION dienstniveau[BeheerEenheid*Dienstniveau] [UNI]

    POPULATION "Dienstniveau" CONTAINS [ "Best effort", "Piket", "Test123" ]
    CLASSIFY Applicatie, Koppeling ISA BeheerEenheid

    INTERFACE Applicatie : I[Applicatie] BOX<FORM>
   [ Applicatie     : I                    CRud
   , dienstniveau   : dienstniveau         cRUd
   ]
ENDCONTEXT

Everything works as expected:

Conclusion

This must be something else. I suspect a browser cache issue. Most of the times a refresh (F5) is all you need. Sometimes you must disable cache. This can be done (in Chrome) using developer tools. See screenshot below. Tick the box Disable cache and then refresh the page (F5).

image

Michiel-s commented 1 year ago

@stefjoosten, please close the issue if this solved your problem. Let me know otherwise.

stefjoosten commented 1 year ago

I'm trying to reproduce it in a smaller setting. Michiel's example will not display the problem I described. So I made following script to show the behavior.

CONTEXT "Issue1387"

    RELATION dienstniveau[BeheerEenheid*Dienstniveau] [UNI]

    POPULATION Dienstniveau CONTAINS [ "Best effort", "Piket", "Test123" ]
    POPULATION BeheerEenheid CONTAINS [ "B5" ]
    POPULATION Applicatie CONTAINS [ "A0" ]
    POPULATION Koppeling CONTAINS [ "K3" ]

    CLASSIFY Applicatie, Koppeling ISA BeheerEenheid

    INTERFACE Overview : "_SESSION";V[SESSION*BeheerEenheid] BOX
    [ beheereenheden : I[BeheerEenheid]
    , applicaties    : I[Applicatie]
    , koppelingen    : I[Koppeling]
    ]

    INTERFACE BeheerEenheid : I[BeheerEenheid] BOX<FORM>
   [ BeheerEenheid   : I                    CRud
   , dienstniveau    : dienstniveau         cRUd
   ]

    INTERFACE Applicatie : I[Applicatie] BOX<FORM>
   [ Applicatie      : I                    CRud
   , dienstniveau    : dienstniveau         cRUd
   ]

    INTERFACE Koppeling : I[Koppeling] BOX<FORM>
   [ Koppeling       : I                    CRud
   , dienstniveau    : dienstniveau         cRUd
   ]
ENDCONTEXT

In RAP, it works as you'd like. Now I'll go back to see if this is a cache issue (again... sigh...)

Michiel-s commented 1 year ago

Regarding the caching, in 3 months time I expect that we can release a beta version of the new frontend. That will solve caching problems.

sjcjoosten commented 1 year ago

Here's a minimal example that is able to reproduce Stef's issue:

CONTEXT "Issue1387"

    RELATION dienstniveau[Applicatie*Dienstniveau]
    RELATION dienstniveau[BeheerEenheid*Dienstniveau] [UNI]

    POPULATION Dienstniveau CONTAINS [ "Best effort", "Piket", "Test123" ]

    CLASSIFY Applicatie ISA BeheerEenheid

    INTERFACE BeheerEenheid : I[BeheerEenheid] BOX<FORM>
   [ BeheerEenheid   : I                    CRud
   , dienstniveau    : dienstniveau         cRUd
   ]

    INTERFACE Applicatie : I[Applicatie] BOX<FORM>
   [ Applicatie      : I                    CRud
   , dienstniveau    : dienstniveau         cRUd
   ]

ENDCONTEXT
Screenshot 2023-02-05 at 09 41 26 Screenshot 2023-02-05 at 09 41 16

Edit: In Stef's larger script, the following additional line that causes the issue was done through the import of an excel sheet:

RELATION dienstniveau[Applicatie*Dienstniveau]
Michiel-s commented 1 year ago

Thanks. I'll have a look

stefjoosten commented 1 year ago

It is not a caching issue. Bas and I have analyzed this as an issue with the xlsx-importer in Haskell. It should not declare relations, but only check whether it can interpret the columns to fit in existing relations. More details later... @Michiel-s , there's nothing wrong with the prototype framework.

Michiel-s commented 1 year ago

Backtracking the isse

For the INTERFACE expression Applicatie > dienstniveau we expect this expression dienstniveau to be UNI, like is the case for BeheerEenheid > dienstniveau.

Apparently the frontend (html template) doesn't get the right input for isUni parameter. How about the backend (json configs)?

Backend config does not get right information from compiler

{
    "NormalizationSteps": [
        "    dienstniveau [Applicatie*Dienstniveau]"
    ],
    "crud": {
        "create": false,
        "delete": false,
        "read": true,
        "update": true
    },
    "expr": {
        "isIdent": false,
        "isTot": false,
        "isUni": false, <-- SHOULD BE TRUE
        "query": "select distinct \"Applicatie\" as src, \"Dienstniveau\" as tgt from \"dienstniveau1\" where (\"Applicatie\" = '_SRCATOM') and ((\"Applicatie\" is not null) and (\"Dienstniveau\" is not null))",
        "srcConceptId": "Applicatie",
        "tgtConceptId": "Dienstniveau"
    },
    "id": "dienstniveau",
    "label": "dienstniveau",
    "relation": "dienstniveau[Applicatie*Dienstniveau]",
    "relationIsFlipped": false,
    "subinterfaces": null,
    "txt": null,
    "type": "ObjExpression",
    "viewId": null
}

Frontend template does not get right information from compiler

The verbose comment for the leave node Applicatie > dienstniveau looks like this. The [UNI] annotation is missing

<!-- Atomic-OBJECT.html "dienstniveau" : dienstniveau [Applicatie*Dienstniveau] :: Applicatie * Dienstniveau   (cRUd) -->

Comparing this with correct annotation for BeheerEenheid > dienstniveau:

<!-- Atomic-OBJECT.html "dienstniveau" : dienstniveau [BeheerEenheid*Dienstniveau] :: BeheerEenheid * Dienstniveau [UNI]  (cRUd) -->

We need to look into the compiler to see where this goes wrong.

Compiler

The isUni attribute for the frontend template generator is set in module Ampersand.Prototype.GenFrontend function buildInterfaces:

return
  FEObjE
    { objName = name object,
      objExp = iExp',
      objCrudC = crudC . objcrud $ object,
      objCrudR = crudR . objcrud $ object,
      objCrudU = crudU . objcrud $ object,
      objCrudD = crudD . objcrud $ object,
      objCrud = objcrud object,
      exprIsUni = isUni . toExpr $ iExp',
      exprIsTot = isTot . toExpr $ iExp',
      relIsProp = case femRelation iExp' of
        Nothing -> False
        Just dcl -> isProp (EDcD dcl),
      exprIsIdent = isIdent . toExpr $ iExp',
      atomicOrBox = aOrB
    }

and for backend files in module Ampersand.Output.ToJSON.Interfaces

instance JSON ObjectDef JSONexpr where
  fromAmpersand env fSpec object =
    JSONexpr
      { exprJSONsrcConceptId = idWithoutType srcConcept,
        exprJSONtgtConceptId = idWithoutType tgtConcept,
        exprJSONisUni = isUni normalizedInterfaceExp,
        exprJSONisTot = isTot normalizedInterfaceExp,
        exprJSONisIdent = isIdent normalizedInterfaceExp,
        exprJSONquery = query
      }

Both get a normalized expression using the function conjNF env $ objExpression object

For me it is hard to diagnose where and how these interface expressions are constructed. Does it has something to do with the normalizer here?

stefjoosten commented 1 year ago

@Michiel-s don't worry, there's nothing wrong with the prototype framework.

Diagnosis

Let us (just for now) call two relations "source-related" if their names are equal and their source concepts are in the same typology. Example:

    RELATION dienstniveau[Applicatie*Dienstniveau]
    RELATION dienstniveau[BeheerEenheid*Dienstniveau] [UNI]
    CLASSIFY Applicatie ISA BeheerEenheid

Ampersand uses two different tables for this. The minimal example can be run in RAP to reproduce the strange behavior. This behavior is correct. If you don't want that behavior, don't define two relations that are different yet source-related.

What happened in my example is that the xlsx-importer defined RELATION dienstniveau[Applicatie*Dienstniveau] for me. Hence my surprise, because I knew no better than the xlsx-importer doesn't define any relations. It turns out it does. That is a bug.

stefjoosten commented 1 year ago

Solution

There are two things to be done:

  1. (must do) Fix the xlsx-importer to define relations ONLY for data-analysis purposes.
  2. (should do) Make a warning if two relations are source-related or target-related because it may yield surprises.

We can discuss whether there is a use case for source-related or target-related relations in the first place. Since their semantics are defined, we need a really good reason to forbid it. But a warning might be in place.

Michiel-s commented 1 year ago

I totally overlooked the double relation definition in the example of @sjcjoosten above.

Very nice that you identified the bug.