Skip to content

chore: Added git resource map types #37746

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.

Sign up for GitHub

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

Merged
nidhi-nair merged 2 commits into from
Nov 27, 2024
Merged

chore: Added git resource map types #37746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
271a15d
chore: Added git resource map types
nidhi-nair Nov 26, 2024
c8f080c
chore: Added git resource map types
nidhi-nair Nov 26, 2024
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
28 changes: 28 additions & 0 deletions ...psmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceIdentity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.appsmith.external.git.models;

import lombok.Data;
import lombok.EqualsAndHashCode;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;

@Data
@RequiredArgsConstructor
public class GitResourceIdentity {
// TODO @Nidhi should we persist the info from parsing this filePath ?
String filePath;
Comment on lines +11 to +12
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the analysis of the codebase, I can generate the final response:

Add @nonnull annotation and remove TODO comment

The filePath field is a critical part of GitResourceIdentity and is used extensively throughout the codebase for file operations, particularly in git-related functionality. The field should never be null as evidenced by its usage in FileOperationsCE interface and its implementations.

  • Add @nonnull annotation to filePath field
  • Remove the TODO comment as the field is clearly used for git operations without persistence needs
🔗 Analysis chain

Address TODO comment and consider field constraints

The TODO comment needs resolution before proceeding. Additionally, consider:

  1. Should filePath be @nonnull?
  2. Should we add validation for the file path format?

Let's analyze the usage patterns of filePath and similar fields to determine if @nonnull is commonly used and how filePath is validated or processed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if filePath is used as required in other files
rg "filePath.*=.*null" --type java

Length of output: 36

Script:

#!/bin/bash
# Check how filePath is used in the codebase
rg "filePath" --type java -A 2

# Check for NonNull annotations in similar model classes
ast-grep --pattern 'class $_ {
  $$$
  @NonNull
  $$$
}'

# Check GitResourceIdentity usage
rg "GitResourceIdentity" --type java -A 2

Length of output: 37266


// TODO @Nidhi should we persist this sha against the Appsmith domain to integrate with the isModified logic?
String sha;
Comment on lines +14 to +15
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider SHA validation and resolve TODO comment

The SHA field might benefit from:

  1. Format validation (git SHA is typically 40 characters)
  2. Clear documentation about the isModified logic integration
-    String sha;
+    @Pattern(regexp = "[0-9a-f]{40}", message = "Must be a valid SHA-1 hash")
+    String sha;

Committable suggestion skipped: line range outside the PR's diff.


@NonNull @EqualsAndHashCode.Include
GitResourceType resourceType;

// This is gitSyncId for most resources, where available
// It could otherwise point to whatever defines uniqueness for that resource
// Exceptions: widgets, jsLibs, json files in root dir
// widgets -> pageGitSyncId-widgetId
// jsLibs -> jsLibFileName
// root dir files -> fileName
@NonNull @EqualsAndHashCode.Include
String resourceIdentifier;
Comment on lines +20 to +27
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for resourceIdentifier formats

The documentation indicates different formats for widgets, jsLibs, and root dir files. Consider adding validation to ensure the format matches the resourceType.

@AssertTrue(message = "Invalid resource identifier format")
private boolean isResourceIdentifierValid() {
    if (resourceType == null || resourceIdentifier == null) {
        return false;
    }
    switch (resourceType) {
        case WIDGET:
            return resourceIdentifier.matches(".*-.*"); // pageGitSyncId-widgetId
        case JS_LIB:
            return resourceIdentifier.matches(".*\\.js$"); // jsLibFileName
        // Add other cases
        default:
            return true;
    }
}

}
15 changes: 15 additions & 0 deletions ...er/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.appsmith.external.git.models;

import com.appsmith.external.dtos.ModifiedResources;
import lombok.Data;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

@Data
public class GitResourceMap {

private Map<GitResourceIdentity, Object> gitResourceMap = new ConcurrentHashMap<>();

private ModifiedResources modifiedResources;
}
21 changes: 21 additions & 0 deletions ...r/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.appsmith.external.git.models;

import java.util.Locale;

public enum GitResourceType {
ROOT_CONFIG,
DATASOURCE_CONFIG,
JSLIB_CONFIG,
PAGE_CONFIG,
JSOBJECT_CONFIG,
JSOBJECT_DATA,
QUERY_CONFIG,
QUERY_DATA,
WIDGET_CONFIG,
;

@Override
public String toString() {
return this.name().toLowerCase(Locale.ROOT);
}
}