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

Issue #418: Change the parameter name on the URL #429

Merged
merged 2 commits into from
Jun 26, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Collaborator

Choose a reason for hiding this comment

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

GPT Review for SecureJsonDataService.java

Review

  • Estimated effort to review [1-5]: 2, because the changes are minor and involve simple refactoring using utility methods. The logic and structure of the code remain largely unchanged.
  • Score: 95

Code feedback

  • File:
    modules_core/com.smf.securewebservices/src/com/smf/securewebservices/service/SecureJsonDataService.java
  • Language:
    java
  • Suggestion:
    Using StringUtils.equals and StringUtils.lowerCase from Apache Commons Lang is a good practice for null-safe operations. However, ensure that the parameters.get method does not return null, or handle the null case explicitly to avoid potential NullPointerException. [important]
  • Label:
    best practice
  • Existing code:
if (StringUtils.equals("true", parameters.get(JsonConstants.SHOW_FK_DROPDOWN_UNFILTERED_PARAMETER))) {
  • Improved code:
if (StringUtils.equals("true", StringUtils.defaultString(parameters.get(JsonConstants.SHOW_FK_DROPDOWN_UNFILTERED_PARAMETER)))) {
  • File:
    modules_core/com.smf.securewebservices/src/com/smf/securewebservices/service/SecureJsonDataService.java
  • Language:
    java
  • Suggestion:
    Ensure that the parameters.get method does not return null, or handle the null case explicitly to avoid potential NullPointerException when using StringUtils.lowerCase. [important]
  • Label:
    best practice
  • Existing code:
final String distinctPropertyPath = StringUtils.lowerCase(parameters.get(JsonConstants.DISTINCT_PARAMETER));
  • Improved code:
final String distinctPropertyPath = StringUtils.lowerCase(StringUtils.defaultString(parameters.get(JsonConstants.DISTINCT_PARAMETER)));

Original file line number Diff line number Diff line change
@@ -390,7 +390,7 @@ private DataEntityQueryService createSetQueryService(Map<String, String> paramet
// this is the main entity of a 'contains' (used in FK drop down lists), it will create also
// info for subentity

if ("true".equals(parameters.get(JsonConstants.SHOW_FK_DROPDOWN_UNFILTERED_PARAMETER))) {
if (StringUtils.equals("true", parameters.get(JsonConstants.SHOW_FK_DROPDOWN_UNFILTERED_PARAMETER))) {
// Do not filter out the rows of the referenced tables if
// they are not referenced from the referencing tables
// Showing the records unfiltered improves the performance if the referenced table has just
@@ -435,7 +435,7 @@ private DataEntityQueryService createSetQueryService(Map<String, String> paramet
removeWhereParameter(parameters);
} else {

final String distinctPropertyPath = parameters.get(JsonConstants.DISTINCT_PARAMETER);
final String distinctPropertyPath = StringUtils.lowerCase(parameters.get(JsonConstants.DISTINCT_PARAMETER));
final Property distinctProperty = DalUtil.getPropertyFromPath(
ModelProvider.getInstance().getEntity(entityName), distinctPropertyPath);
final Entity distinctEntity = distinctProperty.getTargetEntity();
Copy link
Collaborator

Choose a reason for hiding this comment

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

GPT Review for doc.yaml

Review

  • Estimated effort to review [1-5]: 1, because the changes are minimal and straightforward, involving only the renaming of path parameters in a YAML file.
  • Score: 95

Code feedback

  • File:
    modules_core/com.smf.securewebservices/web/com.smf.securewebservices/doc/doc.yaml
  • Language:
    yaml
  • Suggestion:
    Ensure that the new parameter name _entityName is consistently used throughout the codebase and documentation. This includes updating any references in the backend code, frontend code, and any other related documentation. [important]
  • Label:
    best practice
  • Existing code:
  /sws/com.smf.securewebservices.obRest/{modelName}:
  /sws/com.smf.securewebservices.obRest/{modelName}/{recordID}:
  • Improved code:
  /sws/com.smf.securewebservices.obRest/{_entityName}:
  /sws/com.smf.securewebservices.obRest/{_entityName}/{recordID}:

Copy link
Collaborator

Choose a reason for hiding this comment

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

GPT Review for doc.yaml

Review

  • Estimated effort to review [1-5]: 1, because the changes are straightforward and involve minor modifications to the endpoint paths and descriptions in the YAML file.
  • Score: 95

Code feedback

  • File:
    modules_core/com.smf.securewebservices/web/com.smf.securewebservices/doc/doc.yaml
  • Language:
    yaml
  • Suggestion:
    The changes made to the endpoint paths and descriptions are clear and improve the clarity of the API documentation. However, ensure that the changes are consistently applied across all related documentation and code to avoid discrepancies. [important]
  • Label:
    best practice
  • Existing code:
  /sws/com.smf.securewebservices.obRest/{modelName}:
  /sws/com.smf.securewebservices.obRest/{modelName}/{recordID}:
  • Improved code:
  /sws/com.smf.securewebservices.obRest/{_entityName}:
  /sws/com.smf.securewebservices.obRest/{_entityName}/{recordID}:

Original file line number Diff line number Diff line change
@@ -98,7 +98,7 @@ paths:
$ref: "#/components/schemas/LogResp"
401:
description: No auth.
/sws/com.smf.securewebservices.obRest/{modelName}:
/sws/com.smf.securewebservices.obRest/{_entityName}:
get:
summary: List records
security:
@@ -132,10 +132,10 @@ paths:
- name: distinct
in: query
required: false
description: Only filters by entities. Returns the specified entity from the model.
description: Specifies that only unique values will be returned of the described entity.
schema:
type: string
example: Organization
example: organization
- name: identifiers
in: query
required: false
@@ -229,7 +229,7 @@ paths:
application/json:
schema:
$ref: "#/components/schemas/Error"
/sws/com.smf.securewebservices.obRest/{modelName}/{recordID}:
/sws/com.smf.securewebservices.obRest/{_entityName}/{recordID}:
get:
summary: Get a record by ID
security:
Loading
Oops, something went wrong.