Trim the connecting string and add new tests by progxaker · Pull Request #3629 · microsoft/ApplicationInsights-Java

progxaker · 2024-04-08T10:15:51Z

Fix #3628. The original approach does not ignore newline or carriage return.

As I mentioned in the issue, I created the following files and reproduced them in the tests:

(Success) No line terminator

progxaker@pc-name# cat -A applicationinsights-connection-string_no-eol.txt 
InstrumentationKey=<ikey>;IngestionEndpoint=https://eastus-8.in.applicationinsights.azure.com/;LiveEndpoint=https://eastus.livediagnostics.monitor.azure.com/progxaker@pc-name#

(Failed) CRLF line terminator (Windows)

progxaker@pc-name# cat -A applicationinsights-connection-string_windows.txt 
InstrumentationKey=<ikey>;IngestionEndpoint=https://eastus-8.in.applicationinsights.azure.com/;LiveEndpoint=https://eastus.livediagnostics.monitor.azure.com/^M$
progxaker@pc-name#

(Failed) CR line terminator (Mac OS)

progxaker@pc-name# cat -A applicationinsights-connection-string_cr-eol.txt 
InstrumentationKey=<ikey>;IngestionEndpoint=https://eastus-8.in.applicationinsights.azure.com/;LiveEndpoint=https://eastus.livediagnostics.monitor.azure.com/^Mprogxaker@pc-name#

(Failed) LF line terminator (Linux)

progxaker@pc-name# cat -A applicationinsights-connection-string.txt 
InstrumentationKey=<ikey>;IngestionEndpoint=https://eastus-8.in.applicationinsights.azure.com/;LiveEndpoint=https://eastus.livediagnostics.monitor.azure.com$
progxaker@pc-name#

Decisions

At first I wanted to replace line terminators with "", but I decided to use the trim() function.
I prefer to add line terminators to the test file so we don't have to check if the file exists or rewrite it:
```
Writer writer = Files.newBufferedWriter(file.toPath(), UTF_8);
writer.write(CONNECTION_STRING+"\r\n");
writer.close();
```
but if you don't want to add the StandardOpenOption class import, I can rewrite that block.
I'm not quite sure if I should use absolute path or relative path, but I added the tests to the end, thinking the tests would run in order.
I prefer to create three tests instead of one with a loop.

Tests

If we run tests without the fix, we get the following results:

Failed test results

# ./gradlew test                                                                                    
                                                
> Task :agent:agent-tooling:compileTestJava                                                                                  
Note: Some input files use or override a deprecated API.                                          
Note: Recompile with -Xlint:deprecation for details.                                                       
Note: Some input files use unchecked or unsafe operations.                                                                        
Note: Recompile with -Xlint:unchecked for details.                                                                                                                                                
                                                                                                                                                                                                  
> Task :agent:agent-tooling:test                                                                                                                                                                  

FileStringLookupTest > testFileWithCRLineTerminators() FAILED
    org.opentest4j.AssertionFailedError:                                                          
    expected: "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://fake-ingestion-endpoint.example.com/"
     but was: "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://fake-ingestion-endpoint.example.com/
"
        at java.base@17.0.10/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base@17.0.10/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base@17.0.10/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base@17.0.10/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//com.microsoft.applicationinsights.agent.internal.configuration.FileStringLookupTest.testFileWithCRLineTerminators(FileStringLookupTest.java:150)

FileStringLookupTest > testFileWithLFLineTerminators() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: 
      "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://fake-ingestion-endpoint.example.com/"
     but was: 
      "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://fake-ingestion-endpoint.example.com/
      "
        at java.base@17.0.10/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base@17.0.10/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base@17.0.10/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base@17.0.10/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//com.microsoft.applicationinsights.agent.internal.configuration.FileStringLookupTest.testFileWithLFLineTerminators(FileStringLookupTest.java:139)

FileStringLookupTest > testFileWithCRLFLineTerminators() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: 
      "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://fake-ingestion-endpoint.example.com/"
     but was: 
      "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://fake-ingestion-endpoint.example.com/
      "
        at java.base@17.0.10/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base@17.0.10/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base@17.0.10/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base@17.0.10/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//com.microsoft.applicationinsights.agent.internal.configuration.FileStringLookupTest.testFileWithCRLFLineTerminators(FileStringLookupTest.java:128)

...

258 tests completed, 3 failed, 10 skipped

If we run with the tests with the fix, the build will succeed.

The PR is open as a draft, as I'd like to get your opinion on it and make changes if necessary.
(I'm just not a developer, so I might be missing something).

Design discussion issue Ignore newline and carriage return in the connection string file #3628
Changes in public surface reviewed
CHANGELOG.md updated

The original approach does not ignore newline or carriage return, what cause errors that will be described in a GitHub issue.

progxaker · 2024-04-08T10:20:56Z

@microsoft-github-policy-service agree

jeanbisutti · 2024-04-10T14:42:52Z

/spotless

jeanbisutti · 2024-04-10T14:45:06Z

@progxaker Thank you for having reported the issue and created a PR to fix it!

The PR is marked as draft. Is it ready for review?

progxaker · 2024-04-10T14:46:16Z

Hello @jeanbisutti. If there are no crucial mistakes, then yes, I can open for review.

jeanbisutti · 2024-04-10T14:51:10Z

@progxaker It looks good to me!

Could you please run ./gradlew :agent:agent-tooling:spotlessApply to fix a code format issue, and then commit and push?

progxaker · 2024-04-10T15:02:02Z

Done.

jeanbisutti

Could you please accept these modifications to fix other code format issues?

progxaker · 2024-04-10T16:01:06Z

~~Abbreviation in name 'testFileWithCRLFLineTerminators' must contain no more than '1' consecutive capital letters.~~

~~Should I rename the tests to testFileWithCrLfLineTerminators or something like that?~~

Never mind, just didn't refresh the page.

Co-authored-by: Jean Bisutti <jean.bisutti@gmail.com>

jeanbisutti · 2024-04-10T17:09:42Z

@progxaker I don't think this PR will fix #3628. connection-string.txt should only contain one line for the connection string.

progxaker · 2024-04-10T17:38:09Z

@jeanbisutti, but I didn't say we need to support multi-line. There is the problem that even if I specify a single line (see failed files), App Insights reads \n, \r\n, \r as a new line, which causes the errors I provided in the issue.
With this PR, I allow the user to put the connection string in a file and use it without worrying about line terminator configurations.

jeanbisutti · 2024-04-11T14:57:37Z

@progxaker I have reproduced the issue. Thank you!

heyams · 2024-04-11T18:08:07Z

thank you @progxaker!

Trim the connecting string and add new tests

0056d71

The original approach does not ignore newline or carriage return, what cause errors that will be described in a GitHub issue.

progxaker requested review from heyams, jeanbisutti, ramthi and trask as code owners April 8, 2024 10:15

progxaker marked this pull request as draft April 8, 2024 10:16

jeanbisutti marked this pull request as ready for review April 10, 2024 14:51

Fix a code format issue

d199236

jeanbisutti reviewed Apr 10, 2024

View reviewed changes

Apply Jean's suggestions from code review

7200951

Co-authored-by: Jean Bisutti <jean.bisutti@gmail.com>

jeanbisutti approved these changes Apr 11, 2024

View reviewed changes

heyams approved these changes Apr 11, 2024

View reviewed changes

heyams merged commit 75645f1 into microsoft:main Apr 11, 2024

Conversation

progxaker commented Apr 8, 2024

Uh oh!

progxaker commented Apr 8, 2024

Uh oh!

jeanbisutti commented Apr 10, 2024

Uh oh!

jeanbisutti commented Apr 10, 2024

Uh oh!

progxaker commented Apr 10, 2024

Uh oh!

jeanbisutti commented Apr 10, 2024

Uh oh!

progxaker commented Apr 10, 2024

Uh oh!

jeanbisutti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

progxaker commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanbisutti commented Apr 10, 2024

Uh oh!

progxaker commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanbisutti commented Apr 11, 2024

Uh oh!

heyams commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

progxaker commented Apr 10, 2024 •

edited

Loading

progxaker commented Apr 10, 2024 •

edited

Loading