Bug fix for #595 * BeanOutputConverter - replace all Windows line separation with \n. * BeanOutputConverterTest - explicitly check line separation using \n. * Introduce TextBlockAssertion that asserts text blocks with line separation set to \n.

Comment From: tzolov

@szarpul I'm afraid your fix is not multiplatform? have you tested it on Mac, Linux and Wind?

Comment From: szarpul

Hey @tzolov , I tested it only on Windows. Nevertheless, the fix should not affect other platforms as they don't have \r\n line separation.

Comment From: nichozhan

Hi @szarpul & @tzolov , How about using System.lineSeparator() as the target of replacement? I have a similar issue in https://github.com/spring-projects/spring-ai/pull/944 while asserting multi-line string equation.

Comment From: szarpul

@nichozhan I would avoid using System.lineSeparator() at all. The test intention is clear, we want to have just \n as a line separation. On the other hand, System.lineSeparator() produces different results on different OS thus making the test flaky (failing on Windows for instance).

Comment From: szarpul

@tzolov I've tested it on Linux using docker image maven:eclipse-temurin-17 with a command clean package and all tests have passed.

Comment From: markpollack

In the issues that came up around this topic, it seems there are different opinions. Is the intention that everything only has \n no matter what the platform? Is this because the AI models can universally understand \n and the other variations \r\n and \r confuse it? Doesn't that seem unlikely? The other intention I thought was to use the correct newline per platform so that any logging/debugging would appear in an appropriate manner.

If the tests are flaky because they don't use System.lineSeparator() then shouldn't the tests be fixed? My gut feel is that the text should reflect the system line separator, so any replacement if needed would be along the lines of myString.replaceAll("\\r\\n|\\r|\\n", System.lineSeparator())

Thoughts?

Comment From: szarpul

Hey @markpollack thanks for the reply!

If the tests are flaky because they don't use System.lineSeparator() then shouldn't the tests be fixed?

I would say it's the other way around, the tests are flaky because of the use of System.lineSeparator() . Meaning System.lineSeparator() differs between different OS. My change fixes that by defining the desired line separator.

If that's correct that \n is a desired line separator is a different topic I would say. However, right now this is how it's defined, please see the comment in the test:

// validate that output contains \n line endings and I'm just adjsuting the tests.

Comment From: szarpul

Hey @markpollack @tzolov * After the fix done in https://github.com/spring-projects/spring-ai/pull/944, the tests were still not passing on Windows. * Following the fix for https://github.com/spring-projects/spring-ai/pull/944, we should compare text blocks against normalized EOL. * If our goal is to have normalized EOL, then there is no sens to hardcode a \n as the target value in BeanOutputConverterTest.normalizesLineEndingsClassType test.

I have updated the PR.

WDYT?

Comment From: markpollack

Thanks for hanging in there on this. I'll dive back in and take a look. The way this reads now has me a bit confused. It looks like there isn't portability in text blocks across platforms?

The related issue https://github.com/spring-projects/spring-ai/issues/595 is about DEFAULT_METADATA_SEPARATOR use in Documents.

I've created a 'cross-platform' label to track / group better

Comment From: szarpul

Hey @markpollack

To clarify the situation, it all comes to one thing, somwhere in the production code the System.lineSeparator() is used, which produces different result for each OS: * \n UNIX * \r\n WINDOWS *

BeanOutputConverterTest

In the method BeanOutputConverter.generateSchema the System.lineSeparator() is used:

ObjectWriter objectWriter = new ObjectMapper().writer(new DefaultPrettyPrinter() .withObjectIndenter(new DefaultIndenter().withLinefeed(System.lineSeparator())));

ContentFormatterTests

This test runs the DefaultContentFormatter with a DEFAULT_METADATA_SEPARATOR set to System.lineSeparator().

private static final String DEFAULT_METADATA_SEPARATOR = System.lineSeparator();

PromptTemplateTest

PromptTemplate class is using StringTemplate library that probably is also using the System.lineSeparator() unde the hood, as the PromptTemplate.render metod is returning the results with Windows specific line separator.

Wrap up

We could either explicitly specify to have the \n in the production code (don't know exactly how to enforce that for the ST library, but lets assume there is a way) or make our test adjust the line separator to the system separator, like in the preposition in this PR (maybe the TextBlockAssertions could be changed to something else).

WDYT?