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.

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

S3900: ShouldExecute method #6942

Merged
merged 39 commits into from Mar 28, 2023

Conversation

zsolt-kolbay-sonarsource
Copy link
Contributor

Part of #6793
Task 4

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource added the Sprint: SE Short-lived* label for epic MMF-3077 *troll label Mar 16, 2023
@github-actions github-actions bot added this to In progress in Best Kanban Mar 16, 2023
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource force-pushed the Zsolt/S3900-ShouldExecute branch 2 times, most recently from db02661 to 62dbf70 Compare March 17, 2023 09:19
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Mar 17, 2023
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Mar 17, 2023
Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of suggestions.

@sonarcloud
Copy link

sonarcloud bot commented Mar 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Mar 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

ITs: Summary for Automapper, Ember-MM, Net5

Comment on lines -460 to -468
"message": "Refactor this method to add validation of parameter 'converter' before using it.",
"location": {
"uri": "sources\Automapper\src\AutoMapper\Configuration\MappingExpressionBase.cs",
"region": {
"startLine": 477,
"startColumn": 26,
"endLine": 477,
"endColumn": 35
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Lost TP

        public void ConvertUsing(ITypeConverter<TSource, TDestination> converter)
        {
            ConvertUsing(converter.Convert);
        }

"region": {
"startLine": 327,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lost TP

        public void Configure(TypeMap typeMap)
        {
            var destMember = DestinationMember;

            if(destMember.DeclaringType.ContainsGenericParameters)
            {
                destMember = typeMap.DestinationSetters.Single(m => m.Name == destMember.Name);
            }

            var propertyMap = typeMap.FindOrCreatePropertyMapFor(destMember, typeof(TMember) == typeof(object) ? destMember.GetMemberType() : typeof(TMember));

            Apply(propertyMap);
        }

"location": {
"uri": "sources\Automapper\src\AutoMapper\Execution\ExpressionBuilder.cs",
"region": {
"startLine": 232,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lost foreach

"location": {
"uri": "sources\Automapper\src\AutoMapper\QueryableExtensions\QueryMapperVisitor.cs",
"region": {
"startLine": 231,
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like FP to me, we should have learned NotNull on the 1st line.

        public static PropertyMap GetPropertyMap(this IGlobalConfiguration config, MemberInfo sourceMemberInfo, Type destinationMemberType)
        {
            var typeMap = config.CheckIfMapExists(sourceMemberInfo.DeclaringType, destinationMemberType);

            var propertyMap = typeMap.PropertyMaps
                .FirstOrDefault(pm => pm.CanResolveValue &&
                                      pm.SourceMember != null && pm.SourceMember.Name == sourceMemberInfo.Name);

            if (propertyMap == null)
                throw PropertyConfigurationException(typeMap, sourceMemberInfo.Name);   // Noncompliant FP

            return propertyMap;
        }

Comment on lines +7 to +9
"uri": "sources\Ember-MM\Ember.Plugins\PluginManager.cs",
"region": {
"startLine": 343,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting FP, we don't support Object.ReferenceEquals

            public bool Equals(EmberPlugin other)
            {
                if (Object.ReferenceEquals(this, other))
                    return true;

                if (Object.ReferenceEquals(other, null))
                    return false;

                return this.Plugin.GetType() == other.Plugin.GetType();
            }

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

ITs: Dump for Nancy

Comment on lines -9 to -12
"startLine": 40,
"startColumn": 20,
"endLine": 40,
"endColumn": 28
Copy link
Contributor

Choose a reason for hiding this comment

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

Lost TP: We don't run for operators?

        public static implicit operator Func<NancyContext, CancellationToken, Task>(AfterPipeline pipeline)
        {
            return pipeline.Invoke;
        }

Comment on lines -735 to -737
"uri": "sources\Nancy\src\Nancy\DefaultTypeCatalog.cs",
"region": {
"startLine": 36,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lost TP: Delegate invocation, or nested lambda

        public IReadOnlyCollection<Type> GetTypesAssignableTo(Type type, TypeResolveStrategy strategy)
        {
            return this.cache.GetOrAdd(type, t => this.GetTypesAssignableTo(type)).Where(strategy.Invoke).ToArray();
        }

Comment on lines +2347 to +2349
"uri": "sources\Nancy\src\Nancy\Owin\NancyMiddleware.cs",
"region": {
"startLine": 54,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting FP, there is ?? null check before

public static MidFunc UseNancy(NancyOptions options = null)
        {
            options = options ?? new NancyOptions();
            options.Bootstrapper.Initialise();

Comment on lines +269 to +298
"startLine": 32,
"startColumn": 129,
"endLine": 32,
"endColumn": 137
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'response' before using it.",
"location": {
"uri": "sources\Nancy\src\Nancy.Testing\BrowserResponseExtensions.cs",
"region": {
"startLine": 35,
"startColumn": 18,
"endLine": 35,
"endColumn": 26
}
}
},
{
"id": "S3900",
"message": "Refactor this method to add validation of parameter 'response' before using it.",
"location": {
"uri": "sources\Nancy\src\Nancy.Testing\BrowserResponseExtensions.cs",
"region": {
"startLine": 37,
"startColumn": 114,
"endLine": 37,
"endColumn": 122
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting method - FPs and FNs

        public static void ShouldHaveRedirectedTo(this BrowserResponse response, string location, StringComparison stringComparer = StringComparison.Ordinal)
        {
            var validRedirectStatuses = new[]
            {
                HttpStatusCode.MovedPermanently,
                HttpStatusCode.SeeOther,
                HttpStatusCode.TemporaryRedirect
            };

            if (!validRedirectStatuses.Any(x => x == response.StatusCode))   // FN here due to lambda
            {
                throw new AssertException(
                    string.Format("Status code should be one of 'MovedPermanently, SeeOther, TemporaryRedirect', but was {0}.", response.StatusCode));
            }

            if (!response.Headers["Location"].Equals(location, stringComparer))  // TP considering the lambda not being supported
            {
                throw new AssertException(string.Format("Location should have been: {0}, but was {1}", location, response.Headers["Location"]));   // FP here, the `if` should have learned NotNull
            }
        }

@@ -19,19 +19,6 @@
"location": {
"uri": "sources\Nancy\src\Nancy.Validation.DataAnnotations\DataAnnotationsValidatorAdapter.cs",
"region": {
"startLine": 48,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lost TP, delegate reference

        public virtual IEnumerable<ModelValidationRule> GetRules(ValidationAttribute attribute, PropertyDescriptor descriptor)
        {
            yield return new ModelValidationRule(ruleType, attribute.FormatErrorMessage, new [] { descriptor == null ? string.Empty : descriptor.Name });
        }

Comment on lines +20 to +22
"uri": "sources\Nancy\src\Nancy.ViewEngines.DotLiquid\DotLiquidViewEngine.cs",
"region": {
"startLine": 90,
Copy link
Contributor

Choose a reason for hiding this comment

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

This 2nd issue looks like FP, the 1st dereference should have learned NotNull.

        public Response RenderView(ViewLocationResult viewLocationResult, dynamic model, IRenderContext renderContext)
        {
            Template parsed;
            Hash hashedModel;
            HttpStatusCode status;

            try
            {
                // Set the parsed template
                parsed = renderContext.ViewCache.GetOrAdd(
                    viewLocationResult,
                    x =>
                    {
                        using (var reader = viewLocationResult.Contents.Invoke())
                            return Template.Parse(reader.ReadToEnd());
                    });

                hashedModel = Hash.FromAnonymousObject(new
                {
                    Model = new DynamicDrop(model),
                    ViewBag = new DynamicDrop(renderContext.Context.ViewBag)
                });

Comment on lines +44 to +49
"message": "Refactor this method to add validation of parameter 'templateContent' before using it.",
"location": {
"uri": "sources\Nancy\src\Nancy.ViewEngines.Markdown\MarkdownViewengineRender.cs",
"region": {
"startLine": 32,
"startColumn": 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a TP, but why didn't the one above didn't go away?

        public static string RenderMasterPage(string templateContent)
        {
            var second =
               templateContent.Substring(   // Old FP? the `IndexOf` below is called first and should have infered `NotNull`
                   templateContent.IndexOf("<!DOCTYPE html>", StringComparison.OrdinalIgnoreCase), // New TP here
                   templateContent.IndexOf("<body", StringComparison.OrdinalIgnoreCase));

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

ITs: Dump for akka.net

Comment on lines +202 to +204
"uri": "sources\akka.net\src\core\Akka\Actor\ActorRef.Extensions.cs",
"region": {
"startLine": 37,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a new FP, because it raises on extension methods. We generally consider invocations of extension methods safe:

        public static IActorRef GetOrElse(this IActorRef actorRef, Func<IActorRef> elseValue)
        {
            return actorRef.IsNobody() ? elseValue() : actorRef;
        }

Comment on lines -228 to -230
"uri": "sources\akka.net\src\core\Akka\Actor\ActorRefFactoryShared.cs",
"region": {
"startLine": 55,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lost FP, that is good.
Also there's interesting FN above it:

        public static ActorSelection ActorSelection(string path, ActorSystem system, IActorRef lookupRoot)
        {
            var provider = ((ActorSystemImpl)system).Provider;  // FN

Comment on lines +889 to +894
"message": "Refactor this method to add validation of parameter 'extension' before using it.",
"location": {
"uri": "sources\akka.net\src\core\Akka\Actor\Internal\ActorSystemImpl.cs",
"region": {
"startLine": 338,
"startColumn": 34,
Copy link
Contributor

Choose a reason for hiding this comment

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

This FP is interesting, because it's inside lambda after a null check in outer method

        public override object RegisterExtension(IExtensionId extension)
        {
            if (extension == null) return null;

            _extensions.GetOrAdd(extension.ExtensionType, t => new Lazy<object>(() => extension.CreateExtension(this), LazyThreadSafetyMode.ExecutionAndPublication));

            return extension.Get(this);
        }

Comment on lines +2878 to +2883
"message": "Refactor this constructor to avoid using members of parameter 'system' because it could be null.",
"location": {
"uri": "sources\akka.net\src\core\Akka\Serialization\NewtonSoftJsonSerializer.cs",
"region": {
"startLine": 162,
"startColumn": 37,
Copy link
Contributor

Choose a reason for hiding this comment

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

FP after a null check

        public NewtonSoftJsonSerializer(ExtendedActorSystem system, NewtonSoftJsonSerializerSettings settings)
            : base(system)
        {
            Settings = new JsonSerializerSettings
            {
                PreserveReferencesHandling = settings.PreserveObjectReferences
                    ? PreserveReferencesHandling.Objects
                    : PreserveReferencesHandling.None,
                NullValueHandling = NullValueHandling.Ignore,
                DefaultValueHandling = DefaultValueHandling.Ignore,
                MissingMemberHandling = MissingMemberHandling.Ignore,
                ConstructorHandling = ConstructorHandling.AllowNonPublicDefaultConstructor,
                TypeNameHandling = settings.EncodeTypeNames
                    ? TypeNameHandling.All
                    : TypeNameHandling.None,
            };

            if (system != null)
            {
                var settingsSetup = system.Settings.Setup.Get<NewtonSoftJsonSerializerSetup>()
                    .GetOrElse(NewtonSoftJsonSerializerSetup.Create(s => {}));

                settingsSetup.ApplySettings(Settings);
            }

Comment on lines +2943 to +2947
"message": "Refactor this constructor to avoid using members of parameter 'system' because it could be null.",
"location": {
"uri": "sources\akka.net\src\core\Akka\Serialization\Serialization.cs",
"region": {
"startLine": 175,
Copy link
Contributor

Choose a reason for hiding this comment

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

FP inside foreach?

        public Serialization(ExtendedActorSystem system)
        {
            System = system;
            _nullSerializer = new NullSerializer(system);
            AddSerializer("null", _nullSerializer);

            var serializersConfig = system.Settings.Config.GetConfig("akka.actor.serializers").AsEnumerable().ToList(); // True positive here
            var serializerBindingConfig = system.Settings.Config.GetConfig("akka.actor.serialization-bindings").AsEnumerable().ToList();
            var serializerSettingsConfig = system.Settings.Config.GetConfig("akka.actor.serialization-settings");

            _serializerDetails = system.Settings.Setup.Get<SerializationSetup>()
                .Select(x => x.CreateSerializers(system)).GetOrElse(ImmutableHashSet<SerializerDetails>.Empty);

            foreach (var kvp in serializersConfig)
            {
                var serializerTypeName = kvp.Value.GetString();
                var serializerType = Type.GetType(serializerTypeName);
                if (serializerType == null)
                {
                    system.Log.Warning("The type name for serializer '{0}' did not resolve to an actual Type: '{1}'", kvp.Key, serializerTypeName); // FP here, was the state lost after AddSerializer below?
                    continue;
                }

                var serializerConfig = serializerSettingsConfig.GetConfig(kvp.Key);

                var serializer = !serializerConfig.IsNullOrEmpty()
                    ? (Serializer)Activator.CreateInstance(serializerType, system, serializerConfig)
                    : (Serializer)Activator.CreateInstance(serializerType, system);

                AddSerializer(kvp.Key, serializer);
            }

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM, the changed ITs are expected at this stage

@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Mar 28, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 119d33f into feature/SE Mar 28, 2023
30 checks passed
Best Kanban automation moved this from Review approved to Validate Peach Mar 28, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the Zsolt/S3900-ShouldExecute branch March 28, 2023 09:24
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource moved this from Validate Peach to Done in Best Kanban Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sprint: SE Short-lived* label for epic MMF-3077 *troll
Projects
Best Kanban
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants