Closed acetousk closed 1 year ago
jonesde
commented
1 year ago I left one small comment, overall the approach and early changes look good to me. As you get further along in this or if you get stuck let me know, I think it's complex enough that at some point a code read meeting will be helpful.
acetousk
commented
1 year ago I left one small comment, overall the approach and early changes look good to me. As you get further along in this or if you get stuck let me know, I think it's complex enough that at some point a code read meeting will be helpful.
I don't see a comment. Maybe send a link?
acetousk
commented
1 year ago @jonesde I'm running into a couple mind benders and I think it's better to change the API a bit.
updatable or canUpdate works better.Because of these things maybe using something like default-nonpk-update="true/false" and default-nonpk-read="true/false" then in each nonpk field it can be update="true/false" or/and read="true/false" which will override the defaults with a warning on overriding pk fields and lastUpdatedStamp.
jonesde
commented
1 year ago @jonesde I'm running into a couple mind benders and I think it's better to change the API a bit.
- Checking whether something is not immutable isn't immediately obvious what the opposite of something opposite is. I think it's a much better API decision to choose the base form of the word (in this case mutable) and determine whether that's true of false.
- If we're designing this to make it usable for people who aren't programmers by trade, I think mutable isn't as good of an option as writable. In writing the function for determining whether a field of a master detail is possible to write / mutate I wrote this: I think writable makes a lot more sense and requires less explaining. Another thing to consider is whether we should inherit CRUD like syntax for this. Maybe
updatableorcanUpdateworks better.- Using the word exclude isn't clear as to what you're excluding from whether you're excluding from reading or writing the field or just the whole field.
Because of these things maybe using something like
default-nonpk-update="true/false"anddefault-nonpk-read="true/false"then in each nonpk field it can beupdate="true/false"or/andread="true/false"which will override the defaults with a warning on overriding pk fields and lastUpdatedStamp.
These are good paths to think down and consider, but you might be getting into over-thinking territory and/or conflating distinct concepts... something that is 'not immutable' is just 'mutable', and include/exclude is not exactly equivalent to 'read' or 'readable'... but... so much of this depends on context and yes if we change the context we'd need to change terminology used.
The main reason to use 'immutable' as an attribute name with a default of false instead of 'mutable' with a default of true is that defaults of true with a manual override to set to false is often weird. If we're going to have a all-fields-immutable sort of attribute which defaults to false, instead of having all fields always mutable unless individually set to immutable, then we might as well switch the field attribute name from immutable to mutable. Immutable makes sense if all fields are always mutable by default, with the default false pattern, but not so much when the fields might be either... I guess that's what you mean by the 'not immutable' idea being confusing, this is the context?
I remember we talked about an include-all-fields sort of attribute, but I don't remember discussing the same for mutability. That idea would change the design, it changes the context for anything at the field level.
For both of those the conundrum we're going to run into no matter how we do it is that we cannot use more context specific language at the field level because at the entity level (via detail relationship) there is more than one possible context (all fields included or excluded, all fields mutable or not). That means there is no field level default in the schema (XSD), so we don't have schema defaults for fields 'included' or 'mutable'. For included if a detail-field element is specified for a field then it gets included, unless perhaps it has an excluded=true attribute. So that changes the context a bit, but I think that's a fine pattern for include/exclude. For mutable, it's just a straight up flag on the detail-field element with no implication from the presence or absence of the detail-field element for a given field name.
I'm probably getting into over-thinking territory on a lot of this, so trying to keep it simple with what we have already, I took at look at what is currently in your repo at:
It looks like there aren't any changes there yet (or just not pushed). On the detail element there is a @mutable attribute that defaults to false and if true then all fields are mutable... unless they are set as immutable at the field level. If detail.@mutable = false then fields cannot be set to mutable, the whole entity is not mutable. The description might change to clarify that, but currently says "If false relationship is read-only and data submitted will be ignored instead of persisted."
In other words looking at the details and context I think the best approach is to completely ignore the detail-field.@immutable attribute if detail.@mutable = false. In yet other words, if a detail entity is not mutable then none of the fields on that entity can flip it to being mutable. We could do a separate attribute like I mentioned above, the detail.@all-fields-immutable thing, but I don't think that adds much value and it does make it all more confusing. The main scenario it would add value for, an option to flip the default along with an option to flip the override, would be most fields immutable and only a small number mutable. That might come up, but trying to accommodate it is quite ugly... and we have a better pattern for that at the include/exclude level where if only a small number of fields are mutable only that small number of fields would be included which might handle most of the cases that come up where a similar concept with mutability might be useful.
For mutability IMO we should stick with what we have. If we get down the road a year and decide it is a pain for some reason or other then we should consider changing it. I think this is a case where adding complexity before knowing we need it is causing problems so even more reason to leave it out of the MVP, ie until we have significant experience with it and more information to decide with.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
On the include and exclude vs readable idea, we're defining a model that represents a subset of the general entity model so the question is which related entities and fields on those entities should be included or excluded.
There is no concept of readable yet, we could add that but in the design we've talked about so far all fields are always readable. A readable concept would be like the writable, perhaps with a default of all fields readable, and then a field could be defined as mutable/writable but not readable/viewable. I'm not saying I think we should add this, but I hope it demonstrates the distinction between readable and included.
On the terminology, IMO we keep what is there with mutable/immutable and exclude. It is more consistent with other things in the framework (using 'mutable' vs 'updatable'), and the XML files are more developer oriented anyway.
In a No/Lo-Code sort of app the labels exposed to the user could be changed, but I personally don't like that sort of thing anyway. Even if there are visual tools to minimize the user's exposure to syntax and technical details, the nature of what they are building or changing is complex and will require a certain level of intelligence, general education, education with the specific tool and underlying concepts (like a relational database), the business context and data model they are dealing with, and much more.
In other words, I don't see No/Lo-Code sorts of tools as being a simplification or dumbing down of what's underneath it. What seems to have happened over time with these sorts of '4GL' and such tools is that those which are not sufficiently functional die, and this often comes from trying to simplify or dumb down too much.
IMO the better focus, and the one often missed by no/lo-code systems, is to leave all the complexity and power, but put effort into a UI that more easily ties things together, has built in references (for the tool as well as for business artifacts like entities and services), and allows the user to create and modify things without being as error-prone and syntax sensitive as text code.
One other little thought... I don't think 'nonpk' is needed in anything in this case. Yes, those options don't apply to PK fields, but PK fields mean something totally different in this context so you can't ever exclude them and the concept of mutable does not even apply (a PK field is never 'changed', a PK field value represent the identity of a record, so if it 'changes' it's not a record updated, it's something that points to a new record).
I'm not sure how to best describe this... the concept of updating a primary key field does not exist. It's not that they can't be 'updated', it's that if you do then you're not changing a record, you're creating a new record or referring to a different record. PK fields are completely different in nature so these concepts just don't apply to them at all, and there is no reason to name things in a way that makes a distinction that is not there, that cannot be there.
IIRC what we talked about before was something like detail.@include-all-fields which defaults to true. It might be better to use detail.@exclude-all-fields which defaults to false. The name of the attribute as an option would make it more clear that all fields are included by default. So, that's what I'd propose after your comments and thinking through it more: add a detail.@exclude-all-fields attribute that defaults to false.
If that attribute is set to true, then we have the semantics I mentioned above where if there is a detail-field element for a field then it gets included unless it has exclude = true. I think this is more 'natural' than something like not having the presense of a detail-field mean it's included (unless you specify otherwise, which some might want to do for auditing reasons even if technically it's a no-op when exclude-all-fields is true).
Yes, as you mentioned lastUpdatedStamp is a special case too, and different from PK fields as well as different from other non-PK fields. It isn't used to identify a record, but it also isn't really updatable because if you set a lastUpdatedStamp in an EntityValue object and try to do a create or update with that, it will be ignored completely on create (the current time is used) and used only for comparison to the value in the DB on update (if optimistic lock is on). We don't need to worry about that at the level of this tool, we can just ignore mutability settings for the lastUpdatedStamp field... ie treat it like it is used at lower levels for 'optimistic locks' and just always pass it through.
My earlier thought was that it would be default included because all fields will be default included, but that isn't true any more. The treatment now is I think it should always be included unless there is a detail-field element with exclude=true... or in other words even if detail.@exclude-all-fields = true the lastUpdatedStamp should still be included. It's a bit weird, but I think closer to what we want.
I think that's all the logic we need to cover corner cases, not sure... and thanks for bringing up these points for discussion. If you feel strongly on any variation we can discuss it more, or I'd say just go for it and let's see how it looks in the end (a privilege an author always has, one of the benefits of being an author :) ).
acetousk
commented
1 year ago I think that's all the logic we need to cover corner cases, not sure... and thanks for bringing up these points for discussion. If you feel strongly on any variation we can discuss it more, or I'd say just go for it and let's see how it looks in the end (a privilege an author always has, one of the benefits of being an author :) ).
Okay I might just go with my idea and it can be changed later. I think that what you're describing makes sense, but there's an easier way to program it that I think is basically more clear and easier to implement. Although I'll certainly take inspiration from this. I'm also not dead set on it. It's just if I'm going to program stuff, I'll program what I think works best. For merging that back into upstream a more comprehensive comparison can be done.
acetousk
commented
1 year ago @jonesde I'm looking at writing the logic for importing master data. I have a note that says:
input master entity (rest.xml m1 transition -> webfacade handleentityrestcall method -> entityfacadeimpl rest method -> entityautoservicerunner storeEntity method (uses store related [follows relationships] and store recursive) -> )I'm guessing that the storeEntity function in EntityAutoServiceRunner line 309 is where it is defined, but I'm not seeing any information about a MasterDefinition for the entity.
-- Side note -- I tried accessing the rest.xml interface through the swagger UI http://localhost:8080/toolstatic/lib/swagger-ui/index.html?url=http://localhost:8080/rest/master.swagger/mantle.party.Party#/default/get_parties_basic , but it threw:
"errorCode": 403, "errors": "User john.doe is not authorized for View on Entity http://mantle.party.Party"What permission does john.doe need to access the m1 api? -- End Side note --
I'm just a bit lost as to where to find the code that I'm supposed to change and how should I test importing master data.
Thanks for the help
jonesde
commented
1 year ago @jonesde I'm looking at writing the logic for importing master data. I have a note that says:
input master entity (rest.xml m1 transition -> webfacade handleentityrestcall method -> entityfacadeimpl rest method -> entityautoservicerunner storeEntity method (uses store related [follows relationships] and store recursive) -> )I'm guessing that the storeEntity function in EntityAutoServiceRunner line 309 is where it is defined, but I'm not seeing any information about a MasterDefinition for the entity.
Yes, that would be the main place. There is no existing way to specify the master-detail name to use so you'll need to add something for that. The current implementation does not use a master definition at all, just the entity and relationship definitions (using relationship definition level mutability, ie with the relationship.@mutable attribute).
Looking at where the EntityFacadeImpl.rest() method calls the entity auto service, looks like line 1740, it uses ecfi.serviceFacade.sync() and not something like calling the EASR.storeEntity() method.
That leads me back to the thought that I'd also like to support something very simple for the service name to use in a screen transition with a service-call element, so maybe an additional optional separator character in an entity-auto pattern service name like:
update#mantle.party.Party$contact
With that syntax way down at the EASR.storeEntity() method it could parse that out of the service name (and in other places treated at part of the 'noun' of the service name).
Another option would be to use a service parameter with a special name, like _masterDetailName. Yet another option, that I don't think is as good, would be to modify methods and classes to pass it through the service call interfaces (or at least the sync one).
-- Side note -- I tried accessing the rest.xml interface through the swagger UI http://localhost:8080/toolstatic/lib/swagger-ui/index.html?url=http://localhost:8080/rest/master.swagger/mantle.party.Party#/default/get_parties_basic , but it threw:
"errorCode": 403, "errors": "User john.doe is not authorized for View on Entity http://mantle.party.Party"What permission does john.doe need to access the m1 api? -- End Side note --
This is the issue with the direct entity interfaces, they require entity level authz. There is a commented example of an artifact group for all entities that I've used for testing in the MantleSetupData.xml file:
<moqui.security.ArtifactGroupMember artifactGroupId="MANTLE_API" artifactName="mantle\..*"
nameIsPattern="Y" artifactTypeEnumId="AT_ENTITY" inheritAuthz="Y"/>Another way to test is in a screen using a transition like the use case mentioned above with a screen transition.
Here's a fairly rough PR for exporting a Master Data Document. It'd be great if @jonesde could review it and make sure I'm going in the right direction.