v2 -> v4 Migration (#128) #129

vishalhcl-5960 · 2024-01-12T10:53:53Z

v2 -> v4

mattmurp · 2024-01-24T20:19:47Z

src/main/java/com/hcl/appscan/sdk/results/CloudResultsProvider.java

@@ -223,7 +223,8 @@ protected String getReportStatus(String reportId) throws IOException, JSONExcept
 			return FAILED;
 		}

-		String request_url = authProvider.getServer() + String.format(API_REPORT_STATUS, reportId);
+		String request_url = authProvider.getServer() + API_REPORT_STATUS;
+		request_url += "?$top=100&$filter=Id eq " +String.format("%s",reportId)+ "&$count=false";


We should just use a single string with String.format() rather than combining 3 separate strings.

Okay. Now, I am using String.format("?$top=100&$filter=Id eq %s&$count=false",reportId) instead of using multiple strings.

mattmurp · 2024-01-24T20:22:34Z

src/main/java/com/hcl/appscan/sdk/scanners/sast/SASTScan.java

@@ -61,7 +61,7 @@ public void run() throws ScannerException, InvalidTargetException {

 	@Override
 	public String getType() {
-		return SAST;
+		return "Sast";


Is there a reason we changed to a hardcoded string instead of a constant here?

Sure Matt, Created a constant for this string.

mattmurp · 2024-01-24T20:22:44Z

src/main/java/com/hcl/appscan/sdk/scanners/sast/SASTScan.java

@@ -125,7 +125,7 @@ protected void analyzeIR() throws IOException, ScannerException {
 	}

    	protected void submitScan() {
-        	setScanId(getServiceProvider().createAndExecuteScan(STATIC_ANALYZER, getProperties()));
+        	setScanId(getServiceProvider().createAndExecuteScan("Sast", getProperties()));


Is there a reason we changed to a hardcoded string instead of a constant here?

Sure, Created a constant for this string.

mattmurp · 2024-01-24T20:25:31Z

src/main/java/com/hcl/appscan/sdk/scan/IScanServiceProvider.java

+	 * @param params A JSON of scan parameters.
+	 * @return The id of the submitted scan, if successful.  Otherwise, null.
+	 */
+	public String createAndExecuteScanWithJSONParameter(String type, JSONObject params);


This overload should keep the same name "createAndExecuteScan" and just change the type for the second parameter.

Yeah Matt, I tried the same but getting the "Ambiguous Method call" error, as the JSONObject class extends the Java.util.HashMap class.
Ambiguous method call. Both
createAndExecuteScan(String, Map<String, String>) in IScanServiceProvider and
createAndExecuteScan(String, JSONObject) in IScanServiceProvider match

Matt, Any suggestion, How should i handle this issue?

Now, using the existing method as we can call the older method with JSONobject as argument.

mattmurp · 2024-01-24T20:29:29Z

src/main/java/com/hcl/appscan/sdk/scan/IScan.java

@@ -19,7 +20,7 @@ public interface IScan {
 	 * @throws ScannerException if a fatal error occurs in the scan.
 	 * @throws InvalidTargetException if the target is invalid.
 	 */
-	public void run() throws ScannerException, InvalidTargetException;
+	public void run() throws ScannerException, InvalidTargetException, JSONException;


I wouldn't modify the interface here because not all IScan implementations may even use JSON data. Instead, the concrete implementation that could potentially throw a JSONException should catch that exception and throw a ScannerException.

Okay, sure.

mattmurp · 2024-01-24T20:39:01Z

src/main/java/com/hcl/appscan/sdk/http/HttpClient.java

@@ -142,6 +143,31 @@ public HttpResponse post(String url, Map<String, String> headerProperties, Map<S
        return post(url, headerProperties, body);
    }

+    public HttpResponse posts(String url, Map<String, String> headerProperties, JSONObject params)


This new code is almost exactly the same as the other method that it's overloading. I suggest combining the common logic into a private method that both overloads can call rather than duplicating the code.

Yeah Matt, I tried the same but getting the "Ambiguous Method call" error.
Ambiguous method call. Both
post (String, Map<String, String>, Map<String, String>) in HttpClient and
post (String, Map<String, String>, JSONObject) in HttpClient match.

Any suggestion, How should i handle this error?

Now, using the existing method as we can call the older method with JSONobject as argument.

mattmurp · 2024-01-24T21:37:46Z

src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java

+            if(loginExpired() || !verifyApplication(params.get(APP_ID).toString()))
+                return null;
+        } catch (JSONException e) {
+            throw new RuntimeException(e);


We don't want to throw a RuntimeException. We should handle any error conditions just as they are handled in the other overload of this method.

Okay Matt, To handle this condition, I am throwing a new ScannerException().

mattmurp · 2024-01-24T21:38:35Z

src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java

-    private boolean shouldUseV4Api(String type) {
-        return type.equals(CoreConstants.SCA);
+    @Override
+    public String createAndExecuteScanWithJSONParameter(String type, JSONObject params) {


This method has a lot of duplicated code from the existing createAndExecuteScan() method. The duplicated code should be moved to a private method that both of these methods can call, rather than having 2 copies of the same code.

Okay, I can do that. But, in above comments you suggested that "This overload should keep the same name "createAndExecuteScan" and just change the type for the second parameter."
After making the changes as suggested by you, I am getting the "ambiguous method call" error.

* Suggested changes

mattmurp · 2024-01-30T14:06:27Z

src/main/java/com/hcl/appscan/sdk/CoreConstants.java

-	String API_REGIONS					= "/api/v2/Utils/Regions";								//$NON-NLS-1$
-	String API_IS_VALID_URL				= "/api/v2/Scans/IsValidUrl";							//$NON-NLS-1$
-	String API_AUTHENTICATION		        = "/api/V2/Account/IsAuthenticated";					//$NON-NLS-1$
+	String API_REPORT_STATUS			= "/api/v4/Reports";									//$NON-NLS-1$


Any reason these constants need to hard code v4 instead of using API_ENV_LATEST?

Doesn't have any such reason. Even for the v2 APIs we were using the hard code strings only.
Now, Using the constant instead of the hard code values.

mattmurp · 2024-01-30T15:08:56Z

src/main/java/com/hcl/appscan/sdk/scan/IScan.java

Since there are no changes in this file (just whitespace) can you remove the file from the PR?

Okay, I will remove this file from the PR.

mattmurp · 2024-01-30T15:16:40Z

src/main/java/com/hcl/appscan/sdk/scan/CloudScanServiceProvider.java

@@ -51,7 +51,7 @@ public CloudScanServiceProvider(IProgress progress, IAuthenticationProvider auth

    @Override
    public String createAndExecuteScan(String type, Map<String, String> params) {
-        if(loginExpired() || !verifyApplication(params.get(APP_ID)))
+        if(loginExpired() || !verifyApplication(params.get(APP_ID).toString()))


This will result in a NullPointerException if the key APP_ID does not exist in the map. I suggest doing a check for params.has(APP_ID) to avoid this.

Okay Matt. Now, using the params.containsKey(APP_ID) check too.

vishalhcl-5960 requested a review from mattmurp January 12, 2024 10:53

mattmurp reviewed Jan 24, 2024

View reviewed changes

vishalhcl-5960 added 7 commits January 27, 2024 02:26

Suggested changes (#133)

8e5cfe3

* Suggested changes

Update ASEScanServiceProvider.java

bdf2182

Update IScan.java

4cd1196

Update IScanServiceProvider.java

84f79c9

Update IScanServiceProvider.java

480e4e5

Update IScan.java

b7087e5

Merge branch 'master' into v2-->-v4

4a672d0

vishalhcl-5960 requested a review from mattmurp February 1, 2024 16:53

Merge branch 'master' into v2-->-v4

2575cb2

mattmurp approved these changes Feb 6, 2024

View reviewed changes

suggested changes (#135)

24fb274

vishalhcl-5960 merged commit 13cc25a into master Feb 6, 2024
2 checks passed

vishalhcl-5960 deleted the v2-->-v4 branch February 6, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2 -> v4 Migration (#128) #129

v2 -> v4 Migration (#128) #129

vishalhcl-5960 commented Jan 12, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

vishalhcl-5960 Jan 25, 2024

vishalhcl-5960 Feb 1, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

vishalhcl-5960 Feb 1, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

mattmurp Jan 24, 2024

vishalhcl-5960 Jan 25, 2024

mattmurp Jan 30, 2024

vishalhcl-5960 Feb 6, 2024

mattmurp Jan 30, 2024

vishalhcl-5960 Feb 6, 2024

mattmurp Jan 30, 2024

vishalhcl-5960 Feb 6, 2024

v2 -> v4 Migration (#128) #129

v2 -> v4 Migration (#128) #129

Conversation

vishalhcl-5960 commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment