Skip to content

Commit 2e8f49c

Browse files
cdhannachrisbeamable
cdhanna
and
chrisbeamable
authored
make config service more safe (#4517)
* make config service more safe * minor comments --------- Co-authored-by: Chris Hanna <chris@beamable.com>
1 parent 19d0c5e commit 2e8f49c

6 files changed

Lines changed: 312 additions & 23 deletions

File tree

‎cli/cli/App.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ public virtual void Configure(
463463
Commands.AddSingleton(EngineSdkVersionOption.Instance);
464464
Commands.AddSingleton(EngineVersionOption.Instance);
465465
Commands.AddSingleton(IgnoreBeamoIdsOption.Instance);
466-
Commands.AddSingleton<QuietOption>();
466+
Commands.AddSingleton(QuietOption.Instance);
467467
Commands.AddSingleton(PidOption.Instance);
468468
Commands.AddSingleton(HostOption.Instance);
469469
Commands.AddSingleton<LimitOption>();
@@ -490,7 +490,7 @@ public virtual void Configure(
490490
root.AddGlobalOption(provider.GetRequiredService<EngineSdkVersionOption>());
491491
root.AddGlobalOption(provider.GetRequiredService<EngineVersionOption>());
492492
root.AddGlobalOption(provider.GetRequiredService<PidOption>());
493-
root.AddGlobalOption(provider.GetRequiredService<QuietOption>());
493+
root.AddGlobalOption(QuietOption.Instance);
494494
root.AddGlobalOption(provider.GetRequiredService<HostOption>());
495495
root.AddGlobalOption(provider.GetRequiredService<AccessTokenOption>());
496496
root.AddGlobalOption(provider.GetRequiredService<RefreshTokenOption>());
@@ -1100,7 +1100,7 @@ protected virtual Parser GetProgram()
11001100
//in case otel is enabled, check if otel data stored in files is too large
11011101
if (Otel.CliTracesEnabled())
11021102
{
1103-
bool quiet = ctx.BindingContext.ParseResult.GetValueForOption(provider.GetRequiredService<QuietOption>());
1103+
bool quiet = ctx.BindingContext.ParseResult.GetValueForOption(QuietOption.Instance);
11041104

11051105
var configService = provider.GetService<ConfigService>();
11061106
var otelDirectory = configService.ConfigTempOtelDirectoryPath;

‎cli/cli/CommandContext.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ public Option<T> AddOption<T>(Option<T> arg, Action<TArgs, BindingContext, T> bi
475475
protected virtual void BindBaseContext(IServiceProvider provider, TArgs args, BindingContext bindingContext)
476476
{
477477
args.Dryrun = bindingContext.ParseResult.GetValueForOption(provider.GetRequiredService<DryRunOption>());
478-
args.Quiet = bindingContext.ParseResult.GetValueForOption(provider.GetRequiredService<QuietOption>());
478+
args.Quiet = bindingContext.ParseResult.GetValueForOption(QuietOption.Instance);
479479
args.IgnoreStandaloneValidation =
480480
bindingContext.ParseResult.GetValueForOption(provider.GetRequiredService<SkipStandaloneValidationOption>());
481481
}

‎cli/cli/Options/QuietOption.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ namespace cli.Options;
44

55
public class QuietOption : Option<bool>
66
{
7-
public QuietOption() : base("--quiet", () => false, "When true, skip input waiting and use default arguments (or error if no defaults are possible)")
7+
public static QuietOption Instance { get; } = new QuietOption();
8+
private QuietOption() : base("--quiet", () => false, "When true, skip input waiting and use default arguments (or error if no defaults are possible)")
89
{
910
AddAlias("-q");
1011
}

‎cli/cli/Services/ConfigService.cs‎

Lines changed: 170 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,9 @@ private void RunMigrations()
219219
var newConfigFile = GetConfigPath(CFG_FILE_NAME);
220220
if (File.Exists(newConfigFile))
221221
{
222-
var existingConfig = JsonConvert.DeserializeObject<JObject>(File.ReadAllText(newConfigFile!));
222+
var existingContent = LockedRead(newConfigFile!);
223+
224+
var existingConfig = JsonConvert.DeserializeObject<JObject>(existingContent);
223225

224226
var existingConfigVersion = GetConfig(CFG_JSON_FIELD_CLI_VERSION, "0.0.123", existingConfig);
225227
var existingPackageVersion = PackageVersion.FromSemanticVersionString(existingConfigVersion);
@@ -254,7 +256,7 @@ private void RunMigrations()
254256
if (File.Exists(oldConfigFile)) File.Move(oldConfigFile, newConfigFile!);
255257
else return;
256258

257-
var newConfig = JsonConvert.DeserializeObject<JObject>(File.ReadAllText(newConfigFile!));
259+
var newConfig = JsonConvert.DeserializeObject<JObject>(LockedRead(newConfigFile!));
258260

259261
// Check for additional projects file and ignored directory files and move their data over.
260262
{
@@ -818,8 +820,114 @@ public void WriteConfig(Action<JObject> modifier, bool isOverride = false)
818820
: ConfigDirectoryPath;
819821

820822
ReadConfigFile(configFolder, true, false, out var config);
823+
var original = (JObject)config.DeepClone();
821824
modifier(config);
822-
FlushConfig(config, configFolder, !isOverride);
825+
826+
var patch = DiffJObject(original, config);
827+
828+
ReadConfigFile(configFolder, true, false, out var latestConfig);
829+
ApplyDiff(latestConfig, patch);
830+
FlushConfig(latestConfig, configFolder, !isOverride);
831+
832+
833+
// chat-gippity wrote these methods...
834+
JObject DiffJObject(JObject original, JObject modified)
835+
{
836+
var set = new JObject();
837+
var remove = new JArray();
838+
var children = new JObject();
839+
840+
// Detect removed and changed properties
841+
foreach (var prop in original.Properties())
842+
{
843+
var name = prop.Name;
844+
845+
if (!modified.TryGetValue(name, out var newValue))
846+
{
847+
remove.Add(name);
848+
continue;
849+
}
850+
851+
var oldValue = prop.Value;
852+
853+
if (oldValue.Type == JTokenType.Object &&
854+
newValue.Type == JTokenType.Object)
855+
{
856+
var childDiff = DiffJObject((JObject)oldValue, (JObject)newValue);
857+
if (childDiff.HasValues)
858+
{
859+
children[name] = childDiff;
860+
}
861+
}
862+
else if (!JToken.DeepEquals(oldValue, newValue))
863+
{
864+
set[name] = newValue.DeepClone();
865+
}
866+
}
867+
868+
// Detect added properties
869+
foreach (var prop in modified.Properties())
870+
{
871+
if (!original.ContainsKey(prop.Name))
872+
{
873+
set[prop.Name] = prop.Value.DeepClone();
874+
}
875+
}
876+
877+
var diff = new JObject();
878+
879+
if (set.HasValues) diff["$set"] = set;
880+
if (remove.HasValues) diff["$remove"] = remove;
881+
if (children.HasValues) diff["$children"] = children;
882+
883+
return diff;
884+
}
885+
void ApplyDiff(JObject target, JObject diff)
886+
{
887+
if (diff == null || !diff.HasValues)
888+
return;
889+
890+
// Apply removals
891+
if (diff["$remove"] is JArray removeArray)
892+
{
893+
foreach (var item in removeArray)
894+
{
895+
target.Remove(item.ToString());
896+
}
897+
}
898+
899+
// Apply sets
900+
if (diff["$set"] is JObject setObj)
901+
{
902+
foreach (var prop in setObj.Properties())
903+
{
904+
target[prop.Name] = prop.Value.DeepClone();
905+
}
906+
}
907+
908+
// Apply children recursively
909+
if (diff["$children"] is JObject childrenObj)
910+
{
911+
foreach (var childProp in childrenObj.Properties())
912+
{
913+
var childName = childProp.Name;
914+
var childDiff = (JObject)childProp.Value;
915+
916+
if (target[childName] is JObject childTarget)
917+
{
918+
ApplyDiff(childTarget, childDiff);
919+
}
920+
else
921+
{
922+
// If missing or not an object, create a new object
923+
var newChild = new JObject();
924+
ApplyDiff(newChild, childDiff);
925+
target[childName] = newChild;
926+
}
927+
}
928+
}
929+
}
930+
823931

824932
}
825933
public T WriteConfig<T>(string path, [CanBeNull] T newValue, bool isOverride=false)
@@ -1087,7 +1195,7 @@ private void FlushConfig(JObject config, string path, bool createSubDirs)
10871195

10881196
try
10891197
{
1090-
File.WriteAllText(fullPath, json);
1198+
LockedWrite(fullPath, json);
10911199
written = true;
10921200
}
10931201
catch (IOException e)
@@ -1495,6 +1603,41 @@ public static bool TryToFindBeamableFolder(string relativePath, out string resul
14951603
return false;
14961604
}
14971605

1606+
public static void LockedWrite(string path, string content, int allowedAttempts = 10, int retryDelayMs = 25)
1607+
{
1608+
for (var i = 0; i < allowedAttempts; i++)
1609+
{
1610+
try
1611+
{
1612+
using var stream = new FileStream(path, FileMode.Create, FileAccess.Write, FileShare.None);
1613+
using var writer = new StreamWriter(stream);
1614+
writer.Write(content);
1615+
writer.Flush();
1616+
}
1617+
catch (IOException) when (i < allowedAttempts)
1618+
{
1619+
Thread.Sleep(retryDelayMs);
1620+
}
1621+
}
1622+
}
1623+
public static string LockedRead(string path, int allowedAttempts=10, int retryDelayMs=25)
1624+
{
1625+
for (var i = 0; i < allowedAttempts ; i ++)
1626+
{
1627+
try
1628+
{
1629+
using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read);
1630+
using var reader = new StreamReader(stream);
1631+
return reader.ReadToEnd();
1632+
}
1633+
catch (IOException) when (i < allowedAttempts)
1634+
{
1635+
Thread.Sleep(retryDelayMs);
1636+
}
1637+
}
1638+
throw new IOException($"Failed to read file after {allowedAttempts} attempts.");
1639+
}
1640+
14981641
/// <summary>
14991642
/// Takes in a folder path and tries to load
15001643
/// </summary>
@@ -1516,23 +1659,33 @@ private static bool ReadConfigFile(string folderPath, bool isOptional, bool enfo
15161659
return false;
15171660
}
15181661

1519-
var content = File.ReadAllText(fullPath);
1520-
1521-
var read = JsonConvert.DeserializeObject<JObject>(content);
1522-
if (read == null)
1662+
var content = LockedRead(fullPath);
1663+
// var content = File.ReadAllText(fullPath);
1664+
1665+
try
15231666
{
1524-
// do not set the result to a null value, ever.
1525-
1526-
if (isOptional)
1667+
var read = JsonConvert.DeserializeObject<JObject>(content);
1668+
if (read == null)
15271669
{
1528-
return true;
1670+
// do not set the result to a null value, ever.
1671+
1672+
if (isOptional)
1673+
{
1674+
return true;
1675+
}
1676+
1677+
BeamableLogger.LogWarning($"Config file was empty at {fullPath}!");
1678+
return false;
15291679
}
1530-
BeamableLogger.LogWarning($"Config file was empty at {fullPath}!");
1531-
return false;
1532-
}
15331680

1534-
result = read;
1535-
return true;
1681+
result = read;
1682+
return true;
1683+
}
1684+
catch (Exception ex)
1685+
{
1686+
Log.Error(ex.Message);
1687+
throw;
1688+
}
15361689
}
15371690

15381691
/// <summary>

0 commit comments

Comments
 (0)