diff --git a/packages/ZibStack.NET.Aop/src/ZibStack.NET.Aop/Generator/AopParser.cs b/packages/ZibStack.NET.Aop/src/ZibStack.NET.Aop/Generator/AopParser.cs index fecc697..a33bcc8 100644 --- a/packages/ZibStack.NET.Aop/src/ZibStack.NET.Aop/Generator/AopParser.cs +++ b/packages/ZibStack.NET.Aop/src/ZibStack.NET.Aop/Generator/AopParser.cs @@ -164,16 +164,32 @@ public static class AopParser /// Builds the parameter/return-type parts of the method model given a pre-resolved /// set of aspects. Factored out so can reuse it. /// - private static InterceptedMethodModel? BuildMethodModel(IMethodSymbol method, List aspects) + /// + /// When non-null, parameter and return-type attributes from this method are merged + /// with 's own. Used by : + /// the emitted signature has to use the interface's parameter list, but [NoLog] / + /// [Sensitive] / [return: NoLog] are typically declared on the concrete impl — so + /// we need to see attributes from both sides or the proxy logs data the impl + /// marked as redacted. + /// + private static InterceptedMethodModel? BuildMethodModel( + IMethodSymbol method, + List aspects, + IMethodSymbol? attributeOverlay = null) { // Parse parameters var parameters = new List(); - foreach (var param in method.Parameters) + for (int pi = 0; pi < method.Parameters.Length; pi++) { - bool isSensitive = param.GetAttributes() - .Any(a => a.AttributeClass?.ToDisplayString() == SensitiveAttributeName); - bool isNoLog = param.GetAttributes() - .Any(a => a.AttributeClass?.ToDisplayString() == NoLogAttributeName); + var param = method.Parameters[pi]; + var overlayParam = attributeOverlay is not null && pi < attributeOverlay.Parameters.Length + ? attributeOverlay.Parameters[pi] + : null; + + bool isSensitive = HasAttr(param, SensitiveAttributeName) + || (overlayParam is not null && HasAttr(overlayParam, SensitiveAttributeName)); + bool isNoLog = HasAttr(param, NoLogAttributeName) + || (overlayParam is not null && HasAttr(overlayParam, NoLogAttributeName)); bool isComplex = IsComplexType(param.Type); SanitizedTypeModel? sanitizedType = null; @@ -405,34 +421,37 @@ private static IReadOnlyList ExtractMethodTypeParameters(IMe continue; var openMethod = (IMethodSymbol)closedMethod.OriginalDefinition; + var implMethod = implClassSymbol.FindImplementationForInterfaceMember(closedMethod) as IMethodSymbol; var aspects = new List(); var seenAspectTypes = new HashSet(); // Method-level aspects declared on the interface member itself (rare but valid). - CollectAspects(openMethod.GetAttributes(), openMethod, aspects, seenAspectTypes); + // Passing implMethod as the attribute-overlay lets [return: NoLog]/[return: Sensitive] + // declared on the concrete impl flow into the proxy too. + CollectAspects(openMethod.GetAttributes(), openMethod, aspects, seenAspectTypes, implMethod); // Method-level aspects on the concrete implementation of THIS interface member. // This is what makes `[Log]` on an impl method intercept calls made via the // interface reference too. - if (implClassSymbol.FindImplementationForInterfaceMember(closedMethod) is IMethodSymbol implMethod) - CollectAspects(implMethod.GetAttributes(), openMethod, aspects, seenAspectTypes); + if (implMethod is not null) + CollectAspects(implMethod.GetAttributes(), openMethod, aspects, seenAspectTypes, implMethod); // Impl class-level aspects (e.g. `[Log]` on the class) apply to every public method. if (classLevelAspectAttrs.Length > 0) - CollectAspects(classLevelAspectAttrs, openMethod, aspects, seenAspectTypes); + CollectAspects(classLevelAspectAttrs, openMethod, aspects, seenAspectTypes, implMethod); // Apply() rules from IAopConfigurator — match against the IMPL method so that // selectors like ClassesWhere(c => c.Name.StartsWith("Order")) resolve against // the concrete class, not the interface. - if (implClassSymbol.FindImplementationForInterfaceMember(closedMethod) is IMethodSymbol implMethodForApply + if (implMethod is not null && (implClassSymbol.ContainingAssembly as ISourceAssemblySymbol)?.Compilation is { } compilation) { var config = AopConfiguratorParser.ReadAll(compilation); foreach (var rule in config.ApplyRules) { - if (!seenAspectTypes.Contains(rule.AspectFqn) && MatchesApplyRule(rule, implMethodForApply)) - CollectVirtualAspect(rule, openMethod, aspects, seenAspectTypes, compilation); + if (!seenAspectTypes.Contains(rule.AspectFqn) && MatchesApplyRule(rule, implMethod)) + CollectVirtualAspect(rule, openMethod, aspects, seenAspectTypes, compilation, implMethod); } } @@ -441,7 +460,10 @@ private static IReadOnlyList ExtractMethodTypeParameters(IMe aspects.Sort((a, b) => a.Order.CompareTo(b.Order)); - var model = BuildMethodModel(openMethod, aspects); + // attributeOverlay = implMethod so [NoLog]/[Sensitive] on the concrete impl's + // parameters flow through the interface proxy (the proxy otherwise only sees + // interface-declared parameter attributes). + var model = BuildMethodModel(openMethod, aspects, implMethod); if (model != null) methods.Add(model); } @@ -615,12 +637,19 @@ internal static bool MatchesApplyRule(AopConfiguratorParser.ApplyRule rule, IMet /// Creates an AspectInfo from an Apply rule — equivalent to having the attribute /// on the method, but driven from the fluent config. /// + /// + /// Additional symbol whose return-type attributes are merged with + /// 's. Used by so that + /// [return: NoLog] declared on the concrete impl propagates through the + /// synthesized interface proxy. + /// private static void CollectVirtualAspect( AopConfiguratorParser.ApplyRule rule, IMethodSymbol method, List aspects, HashSet seenTypes, - Compilation compilation) + Compilation compilation, + IMethodSymbol? attributeOverlay = null) { if (!seenTypes.Add(rule.AspectFqn)) return; @@ -674,23 +703,45 @@ private static void CollectVirtualAspect( } } + var returnAttrs = method.GetReturnTypeAttributes(); + if (attributeOverlay is not null) + returnAttrs = returnAttrs.AddRange(attributeOverlay.GetReturnTypeAttributes()); + bool sensitiveReturn = returnAttrs.Any(a => a.AttributeClass?.ToDisplayString() == SensitiveAttributeName); + bool noLogReturn = returnAttrs.Any(a => a.AttributeClass?.ToDisplayString() == NoLogAttributeName); + aspects.Add(new AspectInfo( rule.AspectFqn, order: 0, properties: props, handlerTypeName: handlerTypeName, - hasSyncHandler: hasSyncHandler, isAsyncHandler: isAsyncHandler, isAroundHandler: isAroundHandler, isAsyncAroundHandler: isAsyncAroundHandler, - genericAroundTypeArg: genericAroundTypeArg)); + sensitiveReturn: sensitiveReturn, + noLogReturn: noLogReturn, + genericAroundTypeArg: genericAroundTypeArg, + hasSyncHandler: hasSyncHandler)); } + /// + /// Resolution order for aspect properties (highest wins): + /// + /// method-level attribute: [Log(LogParameters=false)] + /// class-level attribute: [Log] on the class + /// b.Apply<LogAttribute>(..., a => a.LogParameters = false) + /// ILogConfigurator.Defaults(d => d.LogParameters = false) + /// hard-coded generator default + /// + /// This is realised by populating from sources + /// (1)–(3) (earlier wins) and InterceptedClassModel.AspectClassData from (4). + /// The P(...) helper in each emitter consults them in the same order. + /// private static void CollectAspects( System.Collections.Immutable.ImmutableArray attributes, IMethodSymbol method, List aspects, - HashSet seenTypes) + HashSet seenTypes, + IMethodSymbol? attributeOverlay = null) { foreach (var attr in attributes) { @@ -779,6 +830,8 @@ private static void CollectAspects( } var returnAttrs = method.GetReturnTypeAttributes(); + if (attributeOverlay is not null) + returnAttrs = returnAttrs.AddRange(attributeOverlay.GetReturnTypeAttributes()); bool sensitiveReturn = returnAttrs.Any(a => a.AttributeClass?.ToDisplayString() == SensitiveAttributeName); bool noLogReturn = returnAttrs.Any(a => a.AttributeClass?.ToDisplayString() == NoLogAttributeName); @@ -799,6 +852,9 @@ private static bool DerivesFromAspectAttribute(INamedTypeSymbol? type) return false; } + private static bool HasAttr(IParameterSymbol param, string attrFqn) => + param.GetAttributes().Any(a => a.AttributeClass?.ToDisplayString() == attrFqn); + /// /// Returns true if a generated interceptor — which lives in a separate `__X_Aop` static /// class and invokes `@this.Method(...)` — can legally call a method with this diff --git a/packages/ZibStack.NET.Log/src/ZibStack.NET.Log/Generator/LogAspectEmitter.cs b/packages/ZibStack.NET.Log/src/ZibStack.NET.Log/Generator/LogAspectEmitter.cs index bba173a..8f0af05 100644 --- a/packages/ZibStack.NET.Log/src/ZibStack.NET.Log/Generator/LogAspectEmitter.cs +++ b/packages/ZibStack.NET.Log/src/ZibStack.NET.Log/Generator/LogAspectEmitter.cs @@ -461,26 +461,58 @@ private static void EmitDictEntries(StringBuilder sb, SanitizedTypeModel model, /// internal sealed class LogClassDataProvider : IClassDataProvider { - public string AttributeFullName => "ZibStack.NET.Log.LogAttribute"; + private const string LogAttributeFqn = "ZibStack.NET.Log.LogAttribute"; + + public string AttributeFullName => LogAttributeFqn; // Compilation isn't plumbed through the IClassDataProvider contract, but the // containing assembly's compilation is reachable via the first syntax reference's // semantic model on the class symbol. Close enough for parser discovery. public IReadOnlyDictionary? ExtractClassData(INamedTypeSymbol classSymbol) { - // Check if class has any [Log] methods - bool hasLog = classSymbol.GetMembers().OfType() - .Any(m => m.GetAttributes().Any(a => a.AttributeClass?.ToDisplayString() == "ZibStack.NET.Log.LogAttribute")); - if (!hasLog) return null; - var compilation = GetCompilation(classSymbol); if (compilation is null) return null; + // [Log] reaches a class through three paths — method-level attribute, + // class-level attribute, or an IAopConfigurator.Apply(...) rule + // matching any of its methods. Historically we only checked the first one, so + // global ILogConfigurator defaults (LogParameters=false etc.) silently dropped + // on the floor for the other two paths — the emitter then fell back to the + // hardcoded `true` and kept logging parameters anyway. + if (!HasAnyLogAspect(classSymbol, compilation)) return null; + var defaults = LogConfiguratorParser.Read(compilation); var data = defaults.ToDefaultsDictionary(); return data.Count > 0 ? data : null; } + private static bool HasAnyLogAspect(INamedTypeSymbol classSymbol, Compilation compilation) + { + if (classSymbol.GetAttributes().Any(IsLogAttr)) + return true; + + foreach (var member in classSymbol.GetMembers()) + { + if (member is IMethodSymbol m && m.GetAttributes().Any(IsLogAttr)) + return true; + } + + var logRules = AopConfiguratorParser.ReadAll(compilation).ApplyRules + .Where(r => r.AspectFqn == LogAttributeFqn) + .ToList(); + if (logRules.Count == 0) return false; + + foreach (var member in classSymbol.GetMembers()) + { + if (member is not IMethodSymbol m) continue; + if (logRules.Any(r => AopParser.MatchesApplyRule(r, m))) return true; + } + return false; + } + + private static bool IsLogAttr(AttributeData a) => + a.AttributeClass?.ToDisplayString() == LogAttributeFqn; + private static Compilation? GetCompilation(INamedTypeSymbol classSymbol) { // Classes reaching this provider are declared in the user's source (the generator diff --git a/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/ApplyLogTests.cs b/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/ApplyLogTests.cs index 23a780e..5b181f6 100644 --- a/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/ApplyLogTests.cs +++ b/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/ApplyLogTests.cs @@ -138,4 +138,51 @@ public void ApplyLog_InterfaceDispatch_ReturnValueLogged() var exitLog = entries.First(e => e.Message.Contains("Exited") && e.Message.Contains("GetBalance")); Assert.Contains("300", exitLog.Message); } + + // ── Apply(selector, configure) — aspect configuration at the rule level ── + // Verifies that the second lambda on b.Apply() actually feeds + // LogParameters=false into the emitted interceptor. Historically the flag + // was ignored because LogClassDataProvider only wired up global defaults + // when the class carried a method-level [Log] attribute. + + [Fact] + public void ApplyLog_ConfigureLambda_LogParametersFalse_OmitsArgs() + { + var svc = new NoParamsService(); + svc.Charge("Alice", 99.99m); + + var entries = _logProvider.AllEntries; + var entryLog = entries.First(e => e.Message.Contains("Entering") && e.Message.Contains("Charge")); + Assert.DoesNotContain("Alice", entryLog.Message); + Assert.DoesNotContain("99.99", entryLog.Message); + } + + // ── [NoLog] parameter via interface dispatch ───────────────────────────── + // Covers both placements of the attribute: on the interface parameter and + // on the impl parameter. With the bug fix, both paths redact the value + // from the proxy's entry log. + + [Fact] + public void NoLogOnInterfaceParam_InterfaceDispatch_Redacted() + { + INoLogIfaceParamService svc = new NoLogIfaceParamServiceImpl(); + _ = svc.DoThing("ok", "topsecret"); + + var entry = _logProvider.AllEntries.First(e => + e.Message.Contains("Entering") && e.Message.Contains("DoThing")); + Assert.Contains("ok", entry.Message); + Assert.DoesNotContain("topsecret", entry.Message); + } + + [Fact] + public void NoLogOnImplParam_InterfaceDispatch_Redacted() + { + INoLogImplParamService svc = new NoLogImplParamServiceImpl(); + _ = svc.DoThing("ok", "topsecret"); + + var entry = _logProvider.AllEntries.First(e => + e.Message.Contains("Entering") && e.Message.Contains("DoThing")); + Assert.Contains("ok", entry.Message); + Assert.DoesNotContain("topsecret", entry.Message); + } } diff --git a/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/Fixtures/LogTestServices.cs b/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/Fixtures/LogTestServices.cs index 931ce89..6ec5fe7 100644 --- a/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/Fixtures/LogTestServices.cs +++ b/packages/ZibStack.NET.Log/tests/ZibStack.NET.Log.Tests/Fixtures/LogTestServices.cs @@ -168,5 +168,47 @@ public void Configure(ZibStack.NET.Aop.IAopBuilder b) .ClassesWhere(c => c.Name.StartsWith("E2e")) .PublicMethods() ); + + // Same class-matcher style but with a configure lambda — exercises the + // "Apply(selector, configure)" overload and verifies that the aspect + // property (LogParameters=false) flows down into the generated interceptor. + b.Apply( + to => to.ClassesWhere(c => c.Name.StartsWith("NoParams")), + a => a.LogParameters = false + ); } } + +// Service whose params should NOT appear in the entry log, because the Apply rule +// carries `a => a.LogParameters = false`. Covers hierarchy level 3 (Apply-config). +public class NoParamsService +{ + public string Charge(string customer, decimal amount) => $"charged-{customer}-{amount}"; +} + +// ── [NoLog] on parameters via interface dispatch ──────────────────────────── +// Two shapes: [NoLog] declared on the interface parameter only, and [NoLog] +// declared on the impl parameter only. Both should redact the value when the +// call is made through the interface (i.e. goes through the synthesized proxy). + +public interface INoLogIfaceParamService +{ + string DoThing(string visible, [NoLog] string secret); +} + +public class NoLogIfaceParamServiceImpl : INoLogIfaceParamService +{ + [Log] + public string DoThing(string visible, string secret) => $"{visible}-{secret}"; +} + +public interface INoLogImplParamService +{ + string DoThing(string visible, string secret); +} + +public class NoLogImplParamServiceImpl : INoLogImplParamService +{ + [Log] + public string DoThing(string visible, [NoLog] string secret) => $"{visible}-{secret}"; +}