Issue #2675 - Money with multiple components: EQL and domain metadata#2676
Open
homedirectory wants to merge 58 commits into2.4.0-SNAPSHOTfrom
Open
Issue #2675 - Money with multiple components: EQL and domain metadata#2676homedirectory wants to merge 58 commits into2.4.0-SNAPSHOTfrom
Money with multiple components: EQL and domain metadata#2676homedirectory wants to merge 58 commits into2.4.0-SNAPSHOTfrom
Conversation
… primitive types This covers primitive types that are not directly used as entity properties. For example, Currency is typically not used for entity properties, but is used for Money.currency.
…ties Make Calculated.Data store calculated property category instead of flags `forTotals` and `implicit` which are directly modelled by categories.
…operties The initial implementation is based on DomainMetadataGenerator. Further changes will refactor domain metadata to remove information about calculated expressions from PropertyMetadata and adjust usages to use the introduced abstraction.
…erties of component types
…tities Align @PersistentType of calculated properties and the properties used in their expressions. This removes the need to calculate currency.
…th Hiberante This better adapts to those cases where amount is null but currency is not. E.g., complex calculated expressions for Money where it is difficult to align nullability of amount and currency. Note that the class Money can be instantiated with null amount, resulting in zero.
… retrieval model construction It is simply not necessary. The only effect it has is on ResultQuery1.enhanceAll, which always adds all sub-properties of component types anyway.
It makes more sense to align the "external" attribute. Although it is very unlikely that there will be a union-typed yield that uses extProp. Construct extProp makes sense only in a sub-query, where it is typically used to express conditions. Yields in sub-queries must procude a scalar value, which a union-typed yield typically is not, as it is expanded into one or more yields by this transformation.
Method Prop1.transformBase can be removed now that we have Prop1.resolveProp.
…ession is not a property The aim of this change is to eliminate the workaround in AbstractSource3.
…ped yields This workaround is no longer necessary as EQL now supports Money-typed yields.
Previously, Money has been compared using compareTo which considers only the amount component. To properly support Money with currency, Money.equals should be used, as it also considers the currency component. This change should NOT be breaking. For Money without currency, Money.equals should produce the same results as Money.compareTo: equal iff compareTo == 0, not equal otherwise. This is because all Money instances that do not have currency attain currency from the default locale, which does not change throughout the lifecycle of an application.
…ing EQL conditions
This method does not fully support Money with currency, and multi-component types in general. No alternative public API is provided as there is no need for it at present. This method is still used for some common value matcher logic, which is acceptable at present, as searching by currency is not a priority feature. Futhermore, searching by currency would likely require a more sophisticated implementation to enable searching by currency symbols, rather than currency codes. Replacing EntityUtils.keyPaths by a different abstraction is out of scope of this issue. This would require access to IDomainMetadata, which is in platform-dao. EntityUtils.keyPaths and its usage by value matchers are in platform-pojo-bl, which complicates access to IDomainMetadata, but does not make it impossible. Also removed the tests that covered the removed method which was not necessary.
IoC, where art though?
This complicates reasoning about the inference algorithm. Also, there is no reason to use money.amount in queries, as just "money" is sufficient for all purposes.
…th Hibernate" This reverts commit c51591c. Currency simply must not be null, in any context. For calculated properties, if currency is required but cannot be inferred, it must have a declared expression. For synthetic entities, if currency is required but cannot be inferred, it must be yielded explicitly.
… for entity properties
…ication The test application deliberately mixes different representations of component types to facilitate testing.
…n metadata model
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolve #2675
To be completed by the pull request creator
This section should be completed with reference to section Preparing PR of the Code and PR reviews wiki page.
Create the pull request as a draft by tapping the dropdown arrow on the 'Create pull request' button under the pull request description (below the text box where this description is being edited) and changing the default
Create pull requesttoDraft pull request.Or, if the pull request has already been created, convert it to draft by tapping the "Convert to draft" link beneath the "Reviewers" section.
A self-review of all changes has been completed, and the changes are in sync with the issue requirements.
Changes to the requirements have been reflected in the issue description.
Any "leftovers" such as sysouts, printing of stack traces, and any other "temporary" code, have been removed.
Minor refactorings, such as renamings, extraction of constants, etc., have been addressed.
Developer documentation (e.g., comments, Javadoc), have been provided where required.
New Java tests have been written or existing tests adjusted, if required, to cover the new functionality.
All existing and new Java tests pass successfully by running them with Maven.
All existing and new Web tests pass successfully.
Changes have been inspected for possible NPE situations, and the changes are sufficiently defensive.
The correct base branch has been selected for these changes to be merged into.
The latest changes from the base branch have already been merged into this feature branch (and tested).
Added a change overview to the issue description or as a wiki page, referenced in the issue description.
Some issues might be very descriptive and serve in place of a wiki page.
In such cases consider adding label
Wiki liketo the issue.This pull request does contain significant changes, the section "Significant changes" below is completed and at least one Senior Software Engineer with the relevant area of expertise has been selected as reviewer.
The
In progresslabel has been removed from the issue.The
Pull requestlabel has been added to the issue.The pull request has been made ready for review by tapping the "Ready for review" button below the list of commits on the pull request page.
Significant changes
This pull request contains significant changes as defined in the wiki page.
Details are as follows:
Domain metadata has been refactored: information about expressions for calculated properties, which has previously been stored in property metadata objects, is now encapsulated in a new abstraction --
ICalculatedPropertyExpressionProvider.This change was necessary because inference of calculated expressions, introduced in this issue, requires domain metadata to be already built. In the previous design this would introduce a circular dependency.
Inference of expressions for component-typed calculated properties has been introduced, but in a limited form.
Only
Money.currencyis supported at present. And its inference logic is implemented withinDefaultCalculatedPropertyExpressionProvider. It could be said to be "hard-coded" as there is currently no extension mechanism to define inference rules for arbitrary component types.The same applies to the introduced ability of declaring expressions for sub-properties of component types. At present, only
Money.currencyis supported.To be completed by the pull request reviewer
This section should be completed with reference to section Performing PR review of the Code and PR reviews wiki page.
The
In progresslabel has been added to the pull request in GitHub.The issue requirements have been read and understood (along with any relevant emails and/or Slack messages).
The correct base branch is specified, and that base branch is up-to-date in the local source.
The issue branch has been checked out locally, and had the base branch merged into it.
All automated tests pass successfully.
Ensure the implementation satisfies the functional requirements.
Ensure that code changes are secure and align with the established coding practices, including code formatting and naming conventions.
Ensure that code changes are documented and covered with automated tests as applicable.
Ensure that code changes are well-suited for informal reasoning.
Ensure that changes are documented for the end-user (a software engineer in the case of TG, or an application user in the case of TG-based applications).
If there are significant changes (described above), special attention has been paid to them.
Marked the task items in section "Significant changes" as completed to indicate that corresponding changes have been reviewed, improved if necessary, and approved.
The issue or issues addressed by the pull request are associated with the relevant release milestone.
To be completed by the pull request reviewer once the changes have been reviewed and accepted
The changes have been merged into the base branch (unless there is a specific request not to do so, e.g., they are to be released to SIT).
The issue branch has been deleted (unless the changes have not been merged - see above, or there is a specific request not to do so).
The
In progresslabel has been removed from the pull request.The
Pull requestlabel has been removed from the issue.