Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Windows Support #429

Open
wants to merge 12 commits into
base: 3.12
Choose a base branch
from
Open

feat: Windows Support #429

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
907d2ce
Remove race condition in AuthRecipeTest.paginationTest()
JeremyEastham Apr 16, 2022
78bc0e9
Make file operations work cross-platform
JeremyEastham Apr 16, 2022
f6d1c09
Major update to CONTRIBUTING.md
JeremyEastham Apr 17, 2022
40165cf
Update CHANGELOG.md
JeremyEastham Apr 17, 2022
07bb149
Fix Utils.setValueInConfig()
JeremyEastham Apr 18, 2022
2ee4d6f
Fix WebserverTest.differentHostNameTest()
JeremyEastham Apr 18, 2022
d99e0f4
Add Project Setup links to `CONTRIBUTING.md`
JeremyEastham Apr 20, 2022
3e295ef
Observe visibility of `.testEnvRunning` before running tests
JeremyEastham Apr 20, 2022
63bc059
Rename `startTestingEnv` script to `startTestEnv`
JeremyEastham Apr 23, 2022
5f653d2
Update `CONTRIBUTING.md`
JeremyEastham Apr 24, 2022
ed1fc78
Update `CONTRIBUTING.md`
JeremyEastham Apr 27, 2022
e597766
Update config regexes in `Config.java`
JeremyEastham Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

- Fixes base_path config option not being observed when running `supertokens list`
- Adds base_path normalization logic
- Adds experimental Windows support via Git Bash and WSL2

## [3.12.1] - 2022-04-02

Expand Down
249 changes: 169 additions & 80 deletions CONTRIBUTING.md
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions src/test/java/io/supertokens/test/AuthRecipeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,11 @@ public void paginationTest() throws Exception {
}

UserInfo user1 = EmailPassword.signUp(process.getProcess(), "[email protected]", "password0");
Thread.sleep(1); // Sleep to remove race condition
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
UserInfo user2 = EmailPassword.signUp(process.getProcess(), "[email protected]", "password1");
Thread.sleep(1);
UserInfo user3 = EmailPassword.signUp(process.getProcess(), "[email protected]", "password2");
Thread.sleep(1);
UserInfo user4 = EmailPassword.signUp(process.getProcess(), "[email protected]", "password3");

{
Expand Down
10 changes: 4 additions & 6 deletions src/test/java/io/supertokens/test/ConfigTest2_6.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import org.junit.rules.TestRule;

import java.io.File;
import java.io.FileNotFoundException;
import java.nio.file.Files;

import static junit.framework.TestCase.assertEquals;
import static junit.framework.TestCase.assertFalse;
Expand Down Expand Up @@ -165,16 +167,12 @@ public void testThatNonTestingConfigValuesThrowErrors() throws Exception {
public void testThatMissingConfigFileThrowsError() throws Exception {
String[] args = { "../" };

ProcessBuilder pb = new ProcessBuilder("rm", "config.yaml");
pb.directory(new File(args[0]));
Process process1 = pb.start();
process1.waitFor();
Files.delete(new File(args[0] + "config.yaml").toPath());

TestingProcess process = TestingProcessManager.start(args);
ProcessState.EventAndException e = process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.INIT_FAILURE);
assertNotNull(e);
assertEquals(e.exception.getMessage(),
"java.io.FileNotFoundException: ../config.yaml (No such file or directory)");
assertTrue(e.exception.getCause() instanceof FileNotFoundException);

process.kill();
assertNotNull(process.checkOrWaitForEvent(PROCESS_STATE.STOPPED));
Expand Down
5 changes: 2 additions & 3 deletions src/test/java/io/supertokens/test/LoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,10 @@ public void testThatSubFoldersAreCreated() throws Exception {
assertFalse(logFile.isDirectory());

} finally {

FileUtils.deleteDirectory(new File("../temp/a"));

process.kill();
assertNotNull(process.checkOrWaitForEvent(PROCESS_STATE.STOPPED));

FileUtils.deleteDirectory(new File("../temp/a"));
}

}
Expand Down
77 changes: 28 additions & 49 deletions src/test/java/io/supertokens/test/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.gson.JsonObject;
import io.supertokens.Main;
import io.supertokens.exceptions.QuitProgramException;
import io.supertokens.pluginInterface.PluginInterfaceTesting;
import io.supertokens.storageLayer.StorageLayer;
import io.supertokens.test.httpRequest.HttpRequestForTesting;
Expand All @@ -31,22 +32,21 @@

import java.io.*;
import java.nio.charset.StandardCharsets;
rishabhpoddar marked this conversation as resolved.
Show resolved Hide resolved
import java.nio.file.Files;
import java.nio.file.*;
import java.util.Arrays;
import java.util.regex.Pattern;

public abstract class Utils extends Mockito {

private static ByteArrayOutputStream byteArrayOutputStream;
private static final String newLine = System.lineSeparator();

public static void afterTesting() {
String installDir = "../";
try {

// remove config.yaml file
ProcessBuilder pb = new ProcessBuilder("rm", "config.yaml");
pb.directory(new File(installDir));
Process process = pb.start();
process.waitFor();
Files.delete(new File(installDir + "config.yaml").toPath()); // Use behavior of rm command

// remove webserver-temp folders created by tomcat
final File webserverTemp = new File(installDir + "webserver-temp");
Expand Down Expand Up @@ -104,23 +104,22 @@ public static void reset() {
PluginInterfaceTesting.isTesting = true;
Main.makeConsolePrintSilent = true;
String installDir = "../";
if (!new File(installDir + ".testEnvRunning").exists())
throw new QuitProgramException(
"Testing environment is not running! Run the startTestingEnv script to start it.");
try {

// if the default config is not the same as the current config, we must reset the storage layer
File ogConfig = new File("../temp/config.yaml");
File currentConfig = new File("../config.yaml");
if (currentConfig.isFile()) {
byte[] ogConfigContent = Files.readAllBytes(ogConfig.toPath());
byte[] currentConfigContent = Files.readAllBytes(currentConfig.toPath());
Path ogConfig = new File(installDir + "temp/config.yaml").toPath();
Path currentConfig = new File(installDir + "config.yaml").toPath();
if (currentConfig.toFile().isFile()) {
byte[] ogConfigContent = Files.readAllBytes(ogConfig);
byte[] currentConfigContent = Files.readAllBytes(currentConfig);
if (!Arrays.equals(ogConfigContent, currentConfigContent)) {
StorageLayer.close();
}
}

ProcessBuilder pb = new ProcessBuilder("cp", "temp/config.yaml", "./config.yaml");
pb.directory(new File(installDir));
Process process = pb.start();
process.waitFor();
Files.copy(ogConfig, currentConfig, StandardCopyOption.REPLACE_EXISTING); // Use behavior of cp command

// in devConfig, it's set to false. However, in config, it's commented. So we comment it out so that it
// mimics production. Refer to https://github.com/supertokens/supertokens-core/issues/118
Expand All @@ -138,46 +137,26 @@ public static void reset() {
System.gc();
}

static void commentConfigValue(String key) throws IOException {
private static void replaceConfigValue(String regex, String newStr) throws IOException {
// we close the storage layer since there might be a change in the db related config.
StorageLayer.close();
Path configPath = new File("../config.yaml").toPath();
String originalFileContent = Files.readString(configPath);
String modifiedFileContent = originalFileContent.replaceAll(regex, newStr);
String normalizedFileContent = modifiedFileContent.replaceAll("\r?\n", newLine); // Normalize line endings
Files.writeString(configPath, normalizedFileContent);
}

String oldStr = "\n((#\\s)?)" + key + "(:|((:\\s).+))\n";
String newStr = "\n# " + key + ":";

StringBuilder originalFileContent = new StringBuilder();
try (BufferedReader reader = new BufferedReader(new FileReader("../config.yaml"))) {
String currentReadingLine = reader.readLine();
while (currentReadingLine != null) {
originalFileContent.append(currentReadingLine).append(System.lineSeparator());
currentReadingLine = reader.readLine();
}
String modifiedFileContent = originalFileContent.toString().replaceAll(oldStr, newStr);
try (BufferedWriter writer = new BufferedWriter(new FileWriter("../config.yaml"))) {
writer.write(modifiedFileContent);
}
}

public static void commentConfigValue(String key) throws IOException {
String find = "\r?\n#?\\s*" + Pattern.quote(key) + ":.*\r?\n";
String replace = newLine + "# " + key + ":" + newLine;
replaceConfigValue(find, replace);
}

public static void setValueInConfig(String key, String value) throws IOException {
// we close the storage layer since there might be a change in the db related config.
StorageLayer.close();

String oldStr = "\n((#\\s)?)" + key + "(:|((:\\s).+))\n";
String newStr = "\n" + key + ": " + value + "\n";
StringBuilder originalFileContent = new StringBuilder();
try (BufferedReader reader = new BufferedReader(new FileReader("../config.yaml"))) {
String currentReadingLine = reader.readLine();
while (currentReadingLine != null) {
originalFileContent.append(currentReadingLine).append(System.lineSeparator());
currentReadingLine = reader.readLine();
}
String modifiedFileContent = originalFileContent.toString().replaceAll(oldStr, newStr);
try (BufferedWriter writer = new BufferedWriter(new FileWriter("../config.yaml"))) {
writer.write(modifiedFileContent);
}
}
String find = "\r?\n#?\\s*" + Pattern.quote(key) + ":.*\r?\n";
String replace = newLine + key + ": " + value + newLine;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change the regex expression? The older one worked just fine. Did the older one cause issues in windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this change is necessary.

Old Regex: "\n((#\\s)?)" + key                + "(:|((:\\s).+))\n"
New Regex: "\r?\n#?\\s*" + Pattern.quote(key) + ":.*\r?\n"

Issues with the Old Regex:

  • It used \n instead of \r?\n, which did not match Windows line endings (\r\n)
  • It only matched comments with a single whitespace character after it
  • It did not match non-commented keys with leading whitespace
  • It was also much harder to read due to unnecessary group nesting and the unnecessary | operator
  • It did not quote the key, so any regex characters in the key would make it fail

When reviewing this, I actually noticed one issue with the New Regex. It did not match commented lines with leading whitespace. I have committed this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So this change would be required in the plugins repos as well. Since they too have a similar function in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see. I was thinking that the plugin repos called io.supertokens.test.Utils.commentConfigValue() and io.supertokens.test.Utils.setValueInConfig() directly.

I was also thinking about renaming setValueInConfig() to setConfigValue() to match commentConfigValue(). If I rename all references in supertokens-core, will any change be needed in other repositories?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should change them in the plugin repos too, just to be consistent.

replaceConfigValue(find, replace);
}

public static TestRule getOnFailure() {
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/io/supertokens/test/VersionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.junit.rules.TestRule;

import java.io.File;
import java.io.FileNotFoundException;

import static junit.framework.TestCase.assertEquals;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -90,8 +91,7 @@ public void versionFileMissingTest() throws Exception {

ProcessState.EventAndException e = process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.INIT_FAILURE);
assertNotNull(e);
assertEquals(e.exception.getMessage(),
"java.io.FileNotFoundException: ../version.yaml (No such file or directory)");
assertTrue(e.exception.getCause() instanceof FileNotFoundException);

process.kill();
assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED));
Expand Down
18 changes: 1 addition & 17 deletions src/test/java/io/supertokens/test/WebserverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.net.ConnectException;
import java.net.InetAddress;
import java.net.SocketTimeoutException;
import java.net.*;
import java.util.HashMap;

import static org.junit.Assert.*;
Expand Down Expand Up @@ -597,26 +595,12 @@ public void differentHostNameTest() throws InterruptedException, IOException, Ht
Utils.setValueInConfig("host", "\"localhost\"");
hello("localhost", "3567");
hello("127.0.0.1", "3567");
try {
hello(inetAddress.getHostAddress(), "3567");
if (!inetAddress.getHostAddress().equals("127.0.0.1")) {
fail();
}
} catch (ConnectException ignored) {
}

Utils.reset();

Utils.setValueInConfig("host", "\"127.0.0.1\"");
hello("localhost", "3567");
hello("127.0.0.1", "3567");
try {
hello(inetAddress.getHostAddress(), "3567");
if (!inetAddress.getHostAddress().equals("127.0.0.1")) {
fail();
}
} catch (ConnectException ignored) {
}
Comment on lines -600 to -619
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing these tests cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was failing because the core was not visible to any of my local IP addresses when hosted on localhost due to my firewall settings. This may also be related to how Windows handles localhost addresses, but documentation is sparse on this issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I would require these tests to be there.. Maybe we can change the code here to not run if the OS is windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but wouldn't the situation be similar on other operating systems as well? If settings are strict (as they probably should be), access to localhost servers would not be visible to LAN addresses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has more or less worked with all linux based systems, mac, CICD, GitHub action and everyone in our dev team.. so i don't think it's too much of a problem. I just want this to also run in CICD / GH action pipeline. It would be OK to have them skipped in local dev env.


Utils.reset();

Expand Down