-
Notifications
You must be signed in to change notification settings - Fork 1k
TESTING/DRAFT Immutable built in types and code generation #3534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Change built in types to readonly structs Update localized text implementation to use JSON text serializer Status code with symbolic names Immutable node ids No boxing for all built in types <= 8 bytes in Variant. Split user identity token and token handling via token handler Add migration guide for breaking changes
| return predefinedNode; | ||
| } | ||
|
|
||
| private ServiceResult OnAddSelfAdminRolePermissions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain the benefit and reasoning of this change?
I dont really get what change made this necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValidateRolePermissions checks whether a node has permissions for a roleId. This happens before even "calling". The ones defined by the standard do not include the SelfAdmin RoleId, so that validation fails because i=0 role id (self admin) has no permission of Call.
It is important to expose the SelfAdmin role on the node so that the role permission check passes, hence this code to inject into RolePermissions and into the UserRolePermissions (if the user has the SelfAdmin RoleId).
My assumption why this is needed now is that the RolePermissions for nodes where not generated by model compiler so there were no roles and no roles means anyone can call without a role, and then all checks where happening via AuthorizationHelper "inside the call callback" later.
Another fix could have been to override ValidateRolePermission method which is virtual. But I think this way is best. But tbd.
Comment out exception throwing for idempotent decoding failure.
|
|
||
| // TODO: this loads opc ua assembly, but this should not be needed. | ||
| // but otherwise encoderfactory currently does not get all types. | ||
| var temp = new AlarmConditionState(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still required?
| throw new InvalidOperationException(Utils.Format( | ||
| "Idempotent 3rd gen decoding failed. Type={0}.", | ||
| encodeableTypeName)); | ||
| // throw new InvalidOperationException(Utils.Format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need this exception anymore we should delete the whole if statement
| ], | ||
| // values to assign | ||
| [structureDefinition.DefaultEncodingId? | ||
| [structureDefinition.DefaultEncodingId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is DefaultEncodingId, complexTypeId, binaryEncodingId and xmlEncodingId now guranteed to be not null?
| return WriteBatchedAsync(requestHeader, nodesToWrite, operationLimit, ct); | ||
|
|
||
| async Task<WriteResponse> WriteBatchedAsync( | ||
| async ValueTask<WriteResponse> WriteBatchedAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least this method uses await, should we really change that to ValueTask?
| - `UserNameIdentityTokenHandler` | ||
| - `X509IdentityTokenHandler` | ||
| - `IssuedIdentityTokenHandler` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a chapter regarding to AsBoxedObject?
| /// <inheritdoc/> | ||
| public void Dispose() | ||
| { | ||
| // TODOL Utils.SilentDispose(m_certificate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we are potential leaking native memory
| @@ -314,7 +314,7 @@ public bool CompareDateTime(DateTime value1, DateTime value2) | |||
| /// <param name="value2">Second Value.</param> | |||
| /// <returns>True in case of equal values. | |||
| /// False or ServiceResultException in case of unequal values.</returns> | |||
| public bool CompareUuid(Uuid value1, Uuid value2) | |||
| public bool CompareGuid(Uuid value1, Uuid value2) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why renamed to CompareGuid?
| @@ -827,19 +822,11 @@ public DateTime GetRandomDateTime() | |||
| } | |||
|
|
|||
| /// <inheritdoc/> | |||
| public Guid GetRandomGuid() | |||
| public Uuid GetRandomGuid() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call ed GetRandomUuid for consistency?
Change built in types to readonly structs
Update localized text implementation to use JSON text serializer
Status code with symbolic names
Immutable node ids - no boxing for int identifiers (in nodeId struct)
No boxing for all built in types <= 8 bytes in Variant.
Split user identity token and token handling via token handler Add migration guide for breaking changes
Proposed changes
Describe the changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Related Issues
<Performance Issue>: IsNullTypeId #3533
Fix complex type system test case LoadAllServerDataTypeSystemsAsync(i=92) #3477
Breaking changes migration guide #3425
Getting NodeState for i=68 (PropertyType) returns an object of type BaseDataVariableTypeState #2969
Change implementation of built in types and code generation to generate efficient code leveraging it. #3537
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...