Skip to content
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

JNG-5103 Add the sub types references #145

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

gaborflorian
Copy link
Contributor

@gaborflorian gaborflorian commented Aug 16, 2023

BugJNG-5103 Container does not navigate to the correct association when the container field is inherited

gaborflorian and others added 18 commits August 16, 2023 16:11
…elations_of_subtypes

# Conflicts:
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/executors/SelectStatementExecutor.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/RdbmsBuilder.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/AttributeMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/ConstantMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/EntityTypeNameMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/FunctionMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/IdAttributeMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/RdbmsMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/SubSelectFeatureMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/SubSelectMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/TypeAttributeMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/mappers/VariableMapper.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsColumn.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsConstant.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsCount.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsEntityTypeName.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsField.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsFunction.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsNamedParameter.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsNavigationFilter.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsOrderBy.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsParameter.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/RdbmsResultSet.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/SqlConverterContext.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/join/RdbmsContainerJoin.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/join/RdbmsCustomJoin.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/join/RdbmsJoin.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/join/RdbmsNavigationJoin.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/join/RdbmsQueryJoin.java
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/model/join/RdbmsTableJoin.java
@robertcsakany robertcsakany self-assigned this Oct 18, 2023
@robertcsakany robertcsakany marked this pull request as ready for review October 18, 2023 23:10
@robertcsakany robertcsakany marked this pull request as draft October 18, 2023 23:10
@robertcsakany robertcsakany marked this pull request as ready for review October 20, 2023 07:05
…elations_of_subtypes

# Conflicts:
#	judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/RdbmsBuilder.java
Copy link

coderabbitai bot commented Oct 20, 2024

Walkthrough

The changes involve modifications to two classes: ContainerJoinProcessor and JoinFactory. In ContainerJoinProcessor, new imports are added, and the join processing logic is restructured to utilize a builder pattern for creating join instances. The JoinFactory class sees updates primarily in the buildPath method, which now uses Optional<EReference> for improved handling of navigation logic and has a reformatted method signature for better readability. Logging statements have also been standardized, enhancing clarity without altering the control flow.

Changes

File Change Summary
.../ContainerJoinProcessor.java Added imports for RdbmsFunction, RdbmsNavigationFilter, and RdbmsTableJoin; updated process method to use RdbmsJoin with builder pattern; enhanced join processing logic with commented-out ancestor join preparation.
.../JoinFactory.java Updated buildPath method signature for readability; changed handling of EReference to use Optional<EReference> for robustness; reformatted logging statements for consistency.

Poem

In the code where joins do play,
A builder's charm has come to stay.
With options bright and paths anew,
Our logic dances, fresh and true.
So hop along, dear code, take flight,
For changes bring us pure delight! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/processor/ContainerJoinProcessor.java (1)

91-160: Address the TODO for JNG-5103: Implement ancestor joins

The commented-out code indicates an intention to handle ancestor joins by casting to ancestor join lists. Implementing this functionality is crucial for ensuring that joins correctly consider type hierarchies, which can improve query accuracy and performance.

I can assist in developing this feature to handle ancestor joins effectively. Would you like me to help implement this functionality or open a new GitHub issue to track this task?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 84fed0c and b0e1572.

📒 Files selected for processing (2)
  • judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/processor/ContainerJoinProcessor.java (3 hunks)
  • judo-runtime-core-query/src/main/java/hu/blackbelt/judo/runtime/core/query/JoinFactory.java (5 hunks)
🧰 Additional context used
🪛 ast-grep
judo-runtime-core-query/src/main/java/hu/blackbelt/judo/runtime/core/query/JoinFactory.java

[warning] 166-167: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (
String.format("Reference with name %s cannot be found.", navigation.getFeatureName()))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 172-174: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (!reference.isDerived(),
String.format("Derived references must be resolved by expression builder: %s (%s)",
reference.getName(), modelAdapter.getFqName(reference.eContainer())))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 166-167: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (
String.format("Reference with name %s cannot be found.", navigation.getFeatureName()))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 172-174: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (!reference.isDerived(),
String.format("Derived references must be resolved by expression builder: %s (%s)",
reference.getName(), modelAdapter.getFqName(reference.eContainer())))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

🔇 Additional comments (8)
judo-runtime-core-query/src/main/java/hu/blackbelt/judo/runtime/core/query/JoinFactory.java (6)

84-88: Improved method signature formatting

The method signature has been reformatted to span multiple lines, which enhances readability without changing functionality. This is a good practice for methods with multiple parameters.


120-121: Consistent logging format

The logging statement has been reformatted to use string concatenation, which aligns with the formatting of other logging statements in the codebase. This change enhances consistency without altering the log output.


137-140: Improved feature conversion formatting

The feature conversion call has been reformatted to span multiple lines, consistent with the earlier method signature change. This improves readability without altering the conversion logic.


153-168: Enhanced reference handling and error reporting

The changes introduce a more robust approach to handling references:

  1. Use of Optional<EReference> for initial reference retrieval.
  2. Fallback mechanism to search for references in all entity types if not found initially.
  3. Updated error message for missing references, providing more specific information.

These improvements enhance the reliability of reference handling and provide better error diagnostics, which aligns well with the PR objective of resolving navigation issues related to inherited container fields.

🧰 Tools
🪛 ast-grep

[warning] 166-167: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (
String.format("Reference with name %s cannot be found.", navigation.getFeatureName()))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 166-167: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (
String.format("Reference with name %s cannot be found.", navigation.getFeatureName()))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


170-175: Improved reference validation checks

The reference validation checks have been updated:

  1. The check for unknown references now uses a more concise format.
  2. The check for derived references has been reformatted for consistency.

These changes maintain the same logical checks while improving code readability and error message clarity.

🧰 Tools
🪛 ast-grep

[warning] 172-174: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (!reference.isDerived(),
String.format("Derived references must be resolved by expression builder: %s (%s)",
reference.getName(), modelAdapter.getFqName(reference.eContainer())))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 172-174: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (!reference.isDerived(),
String.format("Derived references must be resolved by expression builder: %s (%s)",
reference.getName(), modelAdapter.getFqName(reference.eContainer())))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


166-168: Addressing static analysis hints

The static analysis tool has flagged potential security issues related to cookie handling (missing Secure and HttpOnly flags). However, these warnings appear to be false positives:

  1. This code is not related to web applications or cookie handling.
  2. The warnings are likely triggered by the use of String.format(), which is commonly used in cookie-related code but has many other uses as well.

We can safely ignore these warnings as they are not applicable to this context.

Also applies to: 172-175

🧰 Tools
🪛 ast-grep

[warning] 166-167: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (
String.format("Reference with name %s cannot be found.", navigation.getFeatureName()))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 166-167: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (
String.format("Reference with name %s cannot be found.", navigation.getFeatureName()))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/processor/ContainerJoinProcessor.java (2)

68-76: Construction of containerJoin is well-structured

The creation of the containerJoin using the builder pattern enhances code readability and maintainability. All necessary parameters are appropriately set, and the use of RdbmsContainerJoin.builder() consolidates the join's properties effectively.


89-89: Ensure the order of joins in the result list is correct

Adding containerJoin to the result list after processing the simple joins may have ordering implications. Verify whether containerJoin should be added before or after the simple joins to maintain the correct join sequence in the SQL query.

To confirm the join order, you can check the dependencies and execution plan using the following script:

✅ Verification successful

Join order is correct

The containerJoin is the only join being added to the result list in ContainerJoinProcessor.java, ensuring the correct sequence in the SQL query.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Analyze the join order in the generated SQL queries.

# Test: Search for the usage of 'containerJoin' and inspect its position relative to other joins.
rg -A 5 "result\.add\(containerJoin\)"

Length of output: 1068


Script:

#!/bin/bash
# Description: Retrieve all join additions in ContainerJoinProcessor.java to verify their order.

rg "result\.add\(" judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/query/processor/ContainerJoinProcessor.java -A 5

Length of output: 418

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants