search

splitio / java-client

Java SDK client for Split Software
https://split.io
Other
25 stars 18 forks source link

SplitFactoryBuilder does not work in localhost test mode when explicitly configuring a yaml file. #84

Closed salvisbh closed 5 years ago

salvisbh commented 5 years ago

With version 3.1.0 of the java-client it is no longer possible to use SplitFactoryBuilder.local(myTestFilePath) Instead, a new SplitClientConfig instance must be passed to the local - static method. In our CD environment we have to be able to set the path for the local test file, because we cannot store a configuration file in the userhome during the test. No client can be created with configured split.yaml, regardless of whether the path is configured absolute or relative with

SplitClientConfig.builder() .splitFile("Testsplits.yaml") .build();

The following error is written to the log:

java.io.FileNotFoundException: \C:\ieu\Projects\GITHUB\splitio\java-client\client\target\test-classes\split.yaml (The filename, directory name, or volume label syntax is incorrect) at java.io.FileInputStream.open0(Native Method) at java.io.FileInputStream.open(FileInputStream.java:195) at java.io.FileInputStream.<init>(FileInputStream.java:138) at java.io.FileReader.<init>(FileReader.java:72) at io.split.client.YamlLocalhostSplitFile.readOnSplits(YamlLocalhostSplitFile.java:27) at io.split.client.LocalhostSplitFactory.<init>(LocalhostSplitFactory.java:46) at io.split.client.LocalhostSplitFactory.createLocalhostSplitFactory(LocalhostSplitFactory.java:32) at io.split.client.SplitFactoryBuilder.local(SplitFactoryBuilder.java:70) at io.split.client.SplitFactoryBuilderTest.localhostModeWithAbsolutePathYaml(SplitFactoryBuilderTest.java:44)

Test for reproducing the Exception: `public class SplitFactoryBuilderTest {

@Test
public void localhostModeWithRelativePathYaml() throws IOException {

    //-- arrange

    //-- act
    SplitClientConfig clientConfig = SplitClientConfig.builder()
            .splitFile(SplitClientConfig.LOCALHOST_DEFAULT_FILE)
            .build();

    SplitFactory splitFactory = SplitFactoryBuilder.local(clientConfig);
    SplitClient client = splitFactory.client();

    //-- assert
    assertThat(client.getTreatment("user_a", "split_1"), is(equalTo("on")));
}

@Test
public void localhostModeWithAbsolutePathYaml() throws Exception {

    //-- arrange
    URL testTogglesUrl = getClass().getClassLoader().getResource(SplitClientConfig.LOCALHOST_DEFAULT_FILE);
    String absolutePath = Paths.get(testTogglesUrl.toURI()).toFile().getAbsolutePath();

    //-- act
    SplitClientConfig clientConfig = SplitClientConfig.builder()
            .splitFile(absolutePath)
            .build();

    SplitFactory splitFactory = SplitFactoryBuilder.local(clientConfig);
    SplitClient client = splitFactory.client();

    //-- assert
    assertThat(client.getTreatment("user_a", "split_1"), is(equalTo("on")));
}

}`

patricioe commented 5 years ago

@salvisbh we are looking into this and will get back to you shortly.

patricioe commented 5 years ago

In the mean time, I just wanted to make sure you are using the new format:

- split_1:
   keys: user_b
   treatment: 'off'
   config: '{ "size" : 20 }'
- split_1:
   keys: [user_a]
   treatment: 'on'
- split_2:
   keys: user_b
   treatment: 'off'
   config: '{ "size" : 44 }'
- splitWithKeys:
   keys: [user_1, user_2, user_3]
   treatment: 'v1'
   config: '{ "size" : 44 }'
- splitWithNoKeys:
   treatment: 'v2'
   config: '{ "size" : 999 }'

We are in the process of updating the documentation to reflect this new format. Legacy format should still work though.

patricioe commented 5 years ago

Alright, I can confirm the code works just fine for us (the second test you provided is correct:

image

The reason for the failure is that you are providing a location where the yaml file does not exist. The reason being is that you are referencing a file (\C:\ieu\Projects\GITHUB\splitio\java-client\client\target\test-classes\split.yaml) that is part of Split source code but unless you are within the Open Source SplitClient project, the bundle of java SplitClient does not get shipped with the test code. It's only part of the source code.

Download this sample file: https://github.com/splitio/java-client/blob/master/client/src/test/resources/split.yaml , place it somewhere where you can reference it's location, and replace it here:

    @Test
    public void localhostModeWithAbsolutePathYaml() throws Exception {

        //-- arrange
        String file = getClass().getClassLoader().getResource(<ABSOLUTE_LOCATION_OF_THE_FILE>).getFile();

        //-- act
        SplitClientConfig clientConfig = SplitClientConfig.builder()
                .splitFile(file)
                .build();

        SplitFactory splitFactory = SplitFactoryBuilder.local(clientConfig);
        SplitClient client = splitFactory.client();

        //-- assert
        assertThat(client.getTreatment("user_a", "split_1"), is(equalTo("on")));
    }

Let me know how if that works for you.

salvisbh commented 5 years ago

Hello Split-Friends Unfortunately, it seems that the library behaves incorrectly in the Windows environment, as shown by the test cases. I didn't create the test case in my module, but directly in the Split-java-client cloned from Github, because there is no explicit test case for the SplitFactoryBuilder, unlike most other classes. Accordingly the file split.yaml is available locally and in the right structure. To check if split.yaml really exists, I extended the test case. ` @Test public void localhostModeWithAbsolutePathYaml() throws Exception {

    //-- arrange
    URL testTogglesUrl = getClass().getClassLoader().getResource(SplitClientConfig.LOCALHOST_DEFAULT_FILE);
    String absolutePath = Paths.get(testTogglesUrl.toURI()).toFile().getAbsolutePath();

    File yaml = new File(absolutePath);
    boolean yamlExists = yaml.exists();

    //-- act
    SplitClientConfig clientConfig = SplitClientConfig.builder()
            .splitFile(absolutePath)
            .build();

    SplitFactory splitFactory = SplitFactoryBuilder.local(clientConfig);
    SplitClient client = splitFactory.client();

    //-- assert
    Assert.assertTrue(yamlExists);
    assertThat(client.getTreatment("user_a", "split_1"), is(equalTo("on")));

}

` The following screenshot shows that the file exists. splitYaml-exists

I did the tests with both JDK 1.8 and OpenJDK. On the Windows platform, the error always occurs. During debugging we discovered that the constructor of java.io.File is not called correctly in class io.split.client.AbstractLocalhostSplitFile. The empty directory (parent) causes a separator to precede the FQN of split.yaml via fs.getDefaultParent() (File, line 321).

With the following adjustment in AbstractLocalhostSplitFile, at least the test case for the absolute path works correctly on all environments. ` public AbstractLocalhostSplitFile(LocalhostSplitFactory splitFactory, String directory, String fileName) throws IOException { Preconditions.checkNotNull(directory); Preconditions.checkNotNull(fileName);

    _splitFactory = Preconditions.checkNotNull(splitFactory);

    //  If no directory is set, instantiate the file without parent, otherwise the path separator is inserted 
   // before the filename in the java.io.File.File(java.lang.String, java.lang.String) class (see line 319).
    _file = (directory.length() > 0) ?
            new File(directory, fileName) :
            new File(fileName);

   //  Original-Code
   //  _file = new File(directory, fileName);

    _watcher = FileSystems.getDefault().newWatchService();
    _stop = new AtomicBoolean(false);
}

` In summary, what is the real problem?

Version 3.0.x of the java-client had the following method signature for local tests in SplitFactoryBuilder

public static SplitFactory local(String home) throws IOException { return new LocalhostSplitFactory(home); } Version 3.1.x breaks the signature without deprecation and now requires a SplitClientConfig instance instead of a directory. ` /**

The easiest way to solve the problem would probably be to use Builder.splitFile(File splitFile) as parameter and expect a file instead of a string. Then the library would have the ability to determine the type (path, file) and would be more fault tolerant.

In summary, it must be possible to set a path or file name (absolute or relative) for yaml or .split as in version 3.0.x, otherwise a build fails in the continuous delivery pipeline. There is no userhome with source code in the CD pipeline.

salvisbh commented 5 years ago

Incorrectly closed by manipulation. Please excuse my lack of experience with Github.

patricioe commented 5 years ago

Thanks again for the detailed research. We'll look into this again and get back to you.

patricioe commented 5 years ago

@salvisbh we tested your patch under the branch localhost-windows using windows platform.

It passed the build and also our manual test for both default behavior using legacy .split file as well as a custom location using SplitClientConfig.builder().splitFile(...).

I'll be publishing a minor release within 24 hours.

Please take a look and let us know if it fixes the issues you ran into.

patricioe commented 5 years ago

Let me know if this latest release address the issue.

salvisbh commented 5 years ago

Hello Patricio, your new version 3.1.1 runs stable in our Continous Delivery Pipeline. Many thanks for the fix.

maddy2308 commented 5 years ago

I see that this error is fixed but I actually have a similar issue while trying to use split.io in localhost mode. When I try to load the file based on the solution provided above I keep getting the error below.

2019-09-06 09:08:04 INFO [main] LocalhostSplitFactory:39 Starting Split in localhost mode with file at /Users/madhurmehta/Desktop/repo/deejay/deejay-ecommerce-library/out/test/resources/config/split_io_localhost.yaml

java.lang.UnsupportedOperationException: Modifier not supported

at sun.nio.fs.PollingWatchService.register(PollingWatchService.java:112)
at sun.nio.fs.UnixPath.register(UnixPath.java:897)
at io.split.client.AbstractLocalhostSplitFile.registerWatcher(AbstractLocalhostSplitFile.java:54)
at io.split.client.LocalhostSplitFactory.<init>(LocalhostSplitFactory.java:50)
at io.split.client.LocalhostSplitFactory.createLocalhostSplitFactory(LocalhostSplitFactory.java:32)
at io.split.client.SplitFactoryBuilder.local(SplitFactoryBuilder.java:70)
at com.deliverr.deejay.ecommerce.commands.ProcessOrderCreateCommandTest.setUp(ProcessOrderCreateCommandTest.java:109)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.internal.runners.MethodRoadie.runBefores(MethodRoadie.java:133)
at org.junit.internal.runners.MethodRoadie.runBeforesThenTestThenAfters(MethodRoadie.java:96)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.executeTest(PowerMockJUnit44RunnerDelegateImpl.java:294)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTestInSuper(PowerMockJUnit47RunnerDelegateImpl.java:127)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTest(PowerMockJUnit47RunnerDelegateImpl.java:82)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runBeforesThenTestThenAfters(PowerMockJUnit44RunnerDelegateImpl.java:282)
at org.junit.internal.runners.MethodRoadie.runTest(MethodRoadie.java:87)
at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:50)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.invokeTestMethod(PowerMockJUnit44RunnerDelegateImpl.java:207)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.runMethods(PowerMockJUnit44RunnerDelegateImpl.java:146)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$1.run(PowerMockJUnit44RunnerDelegateImpl.java:120)
at org.junit.internal.runners.ClassRoadie.runUnprotected(ClassRoadie.java:34)
at org.junit.internal.runners.ClassRoadie.runProtected(ClassRoadie.java:44)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.run(PowerMockJUnit44RunnerDelegateImpl.java:122)
at org.powermock.modules.junit4.common.internal.impl.JUnit4TestSuiteChunkerImpl.run(JUnit4TestSuiteChunkerImpl.java:106)
at org.powermock.modules.junit4.common.internal.impl.AbstractCommonPowerMockRunner.run(AbstractCommonPowerMockRunner.java:53)
at org.powermock.modules.junit4.PowerMockRunner.run(PowerMockRunner.java:59)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)`

Below is the code that I added to get SplitClient working


URL splitFileURL = getClass().getClassLoader().getResource("./config/split_io_localhost.yaml");
  assert splitFileURL != null;
  String splitFilePath = Paths.get(splitFileURL.toURI()).toFile().getAbsolutePath();
  SplitClientConfig splitClientConfig = SplitClientConfig.builder().splitFile(splitFilePath).build();
  SplitFactory splitFactory = SplitFactoryBuilder.local(splitClientConfig);
  this.splitClient = splitFactory.client();```

Any help at this point will be really appreciated.

https://github.com/splitio/java-client/issues/101
patricioe commented 5 years ago

Hi @maddy2308

Could you give us more details about the environment you are running from? Is it Mac, Linux or windows?