-
Notifications
You must be signed in to change notification settings - Fork 535
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
Fix #3290: Add support for generic regex pattern matching #3328
Changes from 13 commits
225a4fc
638895f
e3fd706
b25e776
d44e015
de1d38f
173fe53
618e380
e3830ac
bb93c0a
f7342c1
51e593b
0d17657
16bb041
325f28c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_binary", "kt_jvm_library", "kt_jvm_test") | ||
load("//scripts:script_assets.bzl", "generate_assets_list_from_text_protos") | ||
|
||
REGEX_PATTERN_CHECK_ASSETS = generate_assets_list_from_text_protos( | ||
name = "script_assets", | ||
file_content_validation_file_names = [ | ||
"file_content_validation_checks", | ||
], | ||
filepath_pattern_validation_file_names = [ | ||
"filename_pattern_validation_checks", | ||
], | ||
) | ||
|
||
kt_jvm_binary( | ||
name = "pattern_validation_check", | ||
testonly = True, | ||
data = REGEX_PATTERN_CHECK_ASSETS, | ||
main_class = "org.oppia.android.scripts.RegexPatternValidationCheck$Companion", | ||
runtime_deps = [":pattern_validation_lib"], | ||
) | ||
|
||
kt_jvm_library( | ||
name = "pattern_validation_lib", | ||
testonly = True, | ||
srcs = glob(["src/java/org/oppia/android/scripts/*.kt"]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid using glob? In general its better to explicitly list down the files rather than giving a generic rule like this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
deps = [ | ||
"//scripts/src/java/org/oppia/android/scripts/proto:file_content_validation_structure_java_proto_lite", | ||
"//scripts/src/java/org/oppia/android/scripts/proto:filename_pattern_validation_structure_java_proto_lite", | ||
], | ||
) | ||
|
||
kt_jvm_test( | ||
name = "test_pattern_validation_check", | ||
testonly = True, | ||
srcs = glob(["src/test/org/oppia/android/scripts/RegexPatternValidationCheckTest.kt"]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here and below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
data = glob(["src/test/testfiles/**"]) + REGEX_PATTERN_CHECK_ASSETS, | ||
test_class = "org.oppia.android.scripts.RegexPatternValidationCheckTest", | ||
deps = [ | ||
":pattern_validation_lib", | ||
"//third_party:org_jetbrains_kotlin_kotlin-test-junit", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
file_content_checks { | ||
filename_regex: ".+?.kt" | ||
prohibited_content_regex: "^import .+?support.+?$" | ||
failure_message: "AndroidX should be used instead of the support library" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
filename_checks { | ||
prohibited_filename_regex: "data/src/main/.+?Activity.kt" | ||
failure_message: "Activities can only be placed in the app module" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
load("//domain:domain_assets.bzl", "gen_binary_proto_from_text") | ||
|
||
def _generate_single_asset_proto_binary(name, proto_file_name, proto_dep_name, proto_type_name): | ||
""" | ||
Converts a single asset text proto to a new binary asset. | ||
|
||
Args: | ||
name: str. The name of this target. | ||
proto_file_name: str. The file name of the text proto under the assets directory that will | ||
be converted. This is assuming to correspond to 'src/main/assets/<name>.textproto' and | ||
will lead to a new generated file called 'src/main/assets/<name>.pb'. | ||
proto_dep_name: str. The name of the proto library under //model that contains the proto | ||
definition being converted to binary. | ||
proto_type_name: str. The name of the proto type being converted in the text proto. This is | ||
assumed to be part of the shared 'model' package. | ||
|
||
Returns: | ||
str. The path to the newly generated binary file. | ||
""" | ||
asset_dir = "assets" | ||
return gen_binary_proto_from_text( | ||
name = "generate_binary_proto_for_text_proto_%s" % name, | ||
input_file = "%s/%s.textproto" % (asset_dir, proto_file_name), | ||
output_file = "%s/%s.pb" % (asset_dir, proto_file_name), | ||
proto_deps = [ | ||
"//scripts/src/java/org/oppia/android/scripts/proto:%s_proto" % proto_dep_name, | ||
], | ||
proto_type_name = "model.%s" % proto_type_name, | ||
) | ||
|
||
def _generate_proto_binary_assets(names, proto_dep_name, proto_type_name, name_prefix): | ||
""" | ||
Converts a list of text proto assets to binary. | ||
|
||
Args: | ||
names: list of str. The list of text proto file names under the assets directory that should | ||
be converted. | ||
proto_dep_name: str. See _generate_single_asset_proto_binary. | ||
proto_type_name: str. See _generate_single_asset_proto_binary. | ||
name_prefix: str. A prefix to attach to the name of this target. | ||
|
||
Returns: | ||
list of str. The list of new proto binary asset files that were generated. | ||
""" | ||
return [ | ||
_generate_single_asset_proto_binary( | ||
name = "%s_%s" % (name_prefix, name), | ||
proto_file_name = name, | ||
proto_dep_name = proto_dep_name, | ||
proto_type_name = proto_type_name, | ||
) | ||
for name in names | ||
] | ||
|
||
def generate_assets_list_from_text_protos( | ||
name, | ||
filepath_pattern_validation_file_names, | ||
file_content_validation_file_names): | ||
""" | ||
Converts multiple lists of text proto assets to binary. | ||
|
||
Args: | ||
name: str. The name of this generation instance. This will be a prefix for derived targets. | ||
filepath_pattern_validation_file_names: list of str. The list of prohibited filepath pattern file names. | ||
file_content_validation_file_names: list of str. The list of prohibited file contents file names. | ||
|
||
Returns: | ||
list of str. The list of new proto binary asset files that were generated. | ||
""" | ||
return _generate_proto_binary_assets( | ||
names = filepath_pattern_validation_file_names, | ||
proto_dep_name = "filename_pattern_validation_structure", | ||
proto_type_name = "FilenameChecks", | ||
name_prefix = name, | ||
) + _generate_proto_binary_assets( | ||
names = file_content_validation_file_names, | ||
proto_dep_name = "file_content_validation_structure", | ||
proto_type_name = "FileContentChecks", | ||
name_prefix = name, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,208 @@ | ||
package org.oppia.android.scripts | ||
|
||
import java.io.File | ||
import java.io.FileInputStream | ||
import org.oppia.android.app.model.FilenameChecks | ||
import org.oppia.android.app.model.FilenameCheck | ||
import org.oppia.android.app.model.FileContentChecks | ||
import org.oppia.android.app.model.FileContentCheck | ||
import kotlin.system.exitProcess | ||
|
||
anandwana001 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
class RegexPatternValidationCheck { | ||
companion object { | ||
@JvmStatic | ||
fun main(vararg args: String) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. name should be more descriptive There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, we don't need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, here it looks like we need it in order to run the kt_jvm_binary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you try to change the method name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method name must be |
||
|
||
val repoPath = args[0] + "/" | ||
|
||
val allowedDirectories = arrayOf( | ||
"app", | ||
"data", | ||
"domain", | ||
"model" , | ||
"testing", | ||
"utility", | ||
) | ||
|
||
val searchFiles = collectSearchFiles(repoPath, allowedDirectories) | ||
|
||
var scriptFailedFlag = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can use more empty line in between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
getFilenameChecks().forEach { | ||
if (checkProhibitedFileNamePattern( | ||
repoPath = repoPath, | ||
searchFiles = searchFiles, | ||
prohibitedFilenameRegexString = it.getProhibitedFilenameRegex(), | ||
errorToShow = it.getFailureMessage() | ||
)) { | ||
scriptFailedFlag = true | ||
} | ||
} | ||
|
||
getFileContentChecks().forEach { | ||
if (checkProhibitedContent( | ||
repoPath = repoPath, | ||
searchFiles = searchFiles, | ||
fileNameRegexString = it.getFilenameRegex(), | ||
prohibitedContentRegexString = it.getProhibitedContentRegex(), | ||
errorToShow = it.getFailureMessage() | ||
)) { | ||
scriptFailedFlag = true | ||
} | ||
} | ||
|
||
if (scriptFailedFlag) { | ||
println("REGEX PATTERN CHECKS FAILED") | ||
exitProcess(1) | ||
} else { | ||
println("REGEX PATTERN CHECKS PASSED") | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add some kdocs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
fun getFilenameChecks(): List<FilenameCheck> { | ||
val fileNamePatternsBinaryFile = | ||
File("scripts/assets/filename_pattern_validation_checks.pb") | ||
val filenameCheckBuilder = FilenameChecks.newBuilder() | ||
val namePatternsObj: FilenameChecks = | ||
FileInputStream(fileNamePatternsBinaryFile).use { | ||
filenameCheckBuilder.mergeFrom(it) | ||
}.build() as FilenameChecks | ||
|
||
return namePatternsObj.getFilenameChecksList() | ||
} | ||
|
||
fun getFileContentChecks(): List<FileContentCheck> { | ||
val fileContentsBinaryFile = | ||
File("scripts/assets/file_content_validation_checks.pb") | ||
val fileContentCheckBuilder = FileContentChecks.newBuilder() | ||
val fileContentsObj: FileContentChecks = | ||
FileInputStream(fileContentsBinaryFile).use { | ||
fileContentCheckBuilder.mergeFrom(it) | ||
}.build() as FileContentChecks | ||
|
||
return fileContentsObj.getFileContentChecksList() | ||
} | ||
|
||
/** | ||
* Checks for a prohibited file naming pattern | ||
* | ||
* @param repoPath the path of the repo. | ||
* @param searchFiles a list of all the files which needs to be checked. | ||
* @param fileNameRegexString filename pattern regex which should not be present | ||
* @param errorToShow error to show incase of failure | ||
* @return [Boolean] check failed or passed | ||
*/ | ||
fun checkProhibitedFileNamePattern( | ||
repoPath: String, | ||
searchFiles: Sequence<File>, | ||
prohibitedFilenameRegexString: String, | ||
errorToShow: String | ||
): Boolean { | ||
var filenamePatternCheckFailedFlag = false | ||
val prohibitedFilenameRegex = prohibitedFilenameRegexString.toRegex() | ||
searchFiles.forEach { | ||
val filePath = it.toString().removePrefix(repoPath) | ||
if (prohibitedFilenameRegex.matches(filePath)) { | ||
logProhibitedFilenameFailure( | ||
filePath = filePath, | ||
errorToShow = errorToShow | ||
) | ||
filenamePatternCheckFailedFlag = true | ||
} | ||
} | ||
return filenamePatternCheckFailedFlag | ||
} | ||
|
||
/** | ||
* Checks for a prohibited file content | ||
* | ||
* @param repoPath the path of the repo. | ||
* @param searchFiles a list of all the files which needs to be checked. | ||
* @param fileNameRegexString filename regex string in which to do the content check | ||
* @param prohibitedContentRegexString regex string which should not be contained in the file | ||
* @param errorToShow error to show incase of failure | ||
* @return [Boolean] check failed or passed | ||
*/ | ||
fun checkProhibitedContent( | ||
repoPath: String, | ||
searchFiles: Sequence<File>, | ||
fileNameRegexString: String, | ||
prohibitedContentRegexString: String, | ||
errorToShow: String | ||
): Boolean { | ||
var contentCheckFailedFlag = false | ||
val fileNameRegex = fileNameRegexString.toRegex() | ||
val prohibitedContentRegex = prohibitedContentRegexString.toRegex() | ||
searchFiles.forEach { | ||
if (fileNameRegex.matches(it.name)) { | ||
var lineNumber = 0 | ||
File(it.toString()).forEachLine { lineString -> | ||
lineNumber++ | ||
if (prohibitedContentRegex.matches(lineString)) { | ||
logProhibitedContentFailure( | ||
lineNumber = lineNumber, | ||
errorToShow = errorToShow, | ||
filePath = it.toString().removePrefix(repoPath) | ||
) | ||
contentCheckFailedFlag = true | ||
} | ||
} | ||
} | ||
} | ||
return contentCheckFailedFlag | ||
} | ||
|
||
/** | ||
* Collects the paths of all the files which are needed to be checked | ||
* | ||
* @param repoPath the path of the repo. | ||
* @param allowedDirectories a list of all the directories which needs to be checked. | ||
* @param exemptionList a list of files which needs to be exempted for this check | ||
* @return [Sequence<File>] all files which needs to be checked. | ||
*/ | ||
fun collectSearchFiles( | ||
repoPath: String, | ||
allowedDirectories: Array<String>, | ||
exemptionList: Array<String> = arrayOf<String>() | ||
): Sequence<File> { | ||
val validPaths = File(repoPath).walk().filter { it -> | ||
checkIfAllowedDirectory( | ||
it.toString().removePrefix(repoPath), | ||
allowedDirectories) | ||
&& it.isFile | ||
&& it.name !in exemptionList | ||
} | ||
return validPaths | ||
} | ||
|
||
fun checkIfAllowedDirectory( | ||
pathString: String, | ||
allowedDirectories: Array<String> | ||
): Boolean { | ||
allowedDirectories.forEach { | ||
if (pathString.startsWith(it)) | ||
return true | ||
} | ||
return false | ||
} | ||
|
||
/** Logs the failures for filename pattern violation */ | ||
fun logProhibitedFilenameFailure( | ||
errorToShow: String, | ||
filePath: String | ||
) { | ||
println("Prohibited filename pattern: [ROOT]/$filePath\n" + | ||
"Failure message: $errorToShow\n") | ||
} | ||
|
||
/** Logs the failures for file content violation */ | ||
fun logProhibitedContentFailure( | ||
lineNumber: Int, | ||
errorToShow: String, | ||
filePath: String) { | ||
println("Prohibited content usage found on line no. $lineNumber\n" + | ||
"File: [ROOT]/$filePath\n" + | ||
"Failure message: $errorToShow\n") | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
load("@rules_java//java:defs.bzl", "java_lite_proto_library") | ||
load("@rules_proto//proto:defs.bzl", "proto_library") | ||
|
||
proto_library( | ||
name = "filename_pattern_validation_structure_proto", | ||
srcs = ["filename_pattern_validation_checks.proto"], | ||
visibility = ["//visibility:public"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need visibility to be public? Again, we like to avoid using public as much as possible. Is it possible to provide a set of subpackages for visibility here and below? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
) | ||
|
||
java_lite_proto_library( | ||
name = "filename_pattern_validation_structure_java_proto_lite", | ||
visibility = ["//visibility:public"], | ||
deps = [":filename_pattern_validation_structure_proto"], | ||
) | ||
|
||
proto_library( | ||
name = "file_content_validation_structure_proto", | ||
srcs = ["file_content_validation_checks.proto"], | ||
visibility = ["//visibility:public"], | ||
) | ||
|
||
java_lite_proto_library( | ||
name = "file_content_validation_structure_java_proto_lite", | ||
visibility = ["//visibility:public"], | ||
deps = [":file_content_validation_structure_proto"], | ||
) |
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.
Do we want to be using actions that are created by individuals? @BenHenning
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.
We are currently using this action in the unit_tests.yml workflow:
oppia-android/.github/workflows/unit_tests.yml
Line 27 in 1cc3fc9