-
Notifications
You must be signed in to change notification settings - Fork 0
Suggested additions to the repo #1
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
base: review
Are you sure you want to change the base?
Conversation
Markdown/CommonTasks/ct1.md
Outdated
At this point it's not uncommon for new or relatively inexperienced developers to create a simple checking method to satisfy the immediate requirement. | ||
|
||
```c# | ||
bool ValidateString(string input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be clearer to throw an exception since this is what the aspect is doing.
Markdown/CommonTasks/ct1.md
Outdated
} | ||
``` | ||
|
||
If the requirement was that the string be no less than 10 characters in length and no more than 16 then undeniably it will produce a result. If at some point further on in the program another similar check is required then it's very easy to see how this might be copied and pasted with a a couple of alterations to match the next requirement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is not clear especially "it will produce a result" but having a more realistic example above would help.
Markdown/CommonTasks/ct1.md
Outdated
{ | ||
try | ||
{ | ||
Console.WriteLine("Enter your Password: "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think the example above (the first one) should align with this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that of course was exactly where I got distracted and made the pull request to the contracts!
Markdown/CommonTasks/ct1.md
Outdated
} | ||
``` | ||
|
||
The benefit of using Metalama to handle such tasks is that they are handled consistently whilst your own code remains concise, easy to read and comprehend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good at this point to say that the same aspect can be used on parameters, properties or return values, with one more example when set on a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok the rewritten ct1 is here. Apologies as my lack of git skills coming to the fore again!!
Common Tasks: Input / Output Validation
Many developers will be familiar with the phrase 'Garbage in, garbage out'. In essence if the input being entered into an application is rubbish one shouldn't be surprised if what comes out is rubbish as well. To avoid this developers' need to ensure that what goes into their application's routines meet acceptable criteria and equally what comes out does the same.
Validation is a task that every developer will face at some point and the approach that they take is more often than not a good indication of their overall development experience.
As an example consider a basic requirement that a given string must fall within a given number of characters.
A new or relatively inexperienced developers might create a simple checking method to satisfy the immediate requirement.
bool ValidateString(string input)
{
return input.length < 10 && input.length > 16;
}
If the requirement was that the string be no less than 10 characters in length and no more than 16 then this very simple validation will at least provide a answer to the basic question 'Does this string fall within the defined character length?' However it doesn't really handle failure. Over time developers' will learn how to approach this properly but they will still find themselves having to take differing approaches depending on whether they are validating parameter inputs, properties or results.
Using Metalama tasks like this can be solved easily. It has a patterns library that itself has a number of pre-made contracts for a wide range of scenarios including the example under discussion.
You could write code as follows (using a simple console application as an example) employing no more than a simple attribute;
using Metalama.Patterns.Contracts;
namespace CommonTasks
{
internal class Program
{
[StringLength(10, 16)]
private static string? password;
static void Main(string[] args)
{
try
{
Console.WriteLine("Enter your Password: ");
password = Console.ReadLine();
Console.WriteLine("Your password meets the basic length requirement.");
} catch(ArgumentException ex)
{
Console.WriteLine(ex.Message);
}
}
}
}
Metalama's StringLength aspect takes as parameters either a maximum, or a minimum and maximum length and in the event of a validation failure throws a System.ArgumentException.
At the point of compilation it adds the necessary logic into your code.
using Metalama.Patterns.Contracts;
namespace CommonTasks
{
internal class Program
{
private static string? _password1;
[StringLength(10, 16)]
private static string? password
{
get
{
return Program._password1;
}
set
{
if (value != null && (value!.Length < 10 || value.Length > 16))
{
throw new ArgumentException($"The 'password' property must be a string with length between {10} and {16}.", "value");
}
Program._password1 = value;
}
}
static void Main(string[] args)
{
try
{
Console.WriteLine("Enter your Password: ");
password = Console.ReadLine();
Console.WriteLine("Your password meets the basic length requirement.");
}
catch (ArgumentException ex)
{
Console.WriteLine(ex.Message);
}
}
}
}
There are numerous benefits to using Metalama contracts for validation. They are named in such a way that their intention is clear and where appropriate accept parameters that provide flexibility in the rules being tested. When validation fails it does so by throwing standard exceptions that are easy to handle. The real benefit though is that they can be used in exactly the same way to validate both inputs and outputs.
In the two examples that follow the task remains the same but instead of validating a property input parameters are validated in the first example , and the actual output in the second. In both cases the code that Metalama adds at compilation is also shown.
static string CreatePasswordValidatingInputs([StringLength(5,8)]string a, [StringLength(5, 8)] string b)
{
return a + b;
}
Which at compile time becomes;
static string CreatePasswordValidatingInputs([StringLength(5,8)]string a, [StringLength(5, 8)] string b)
{
if (b.Length < 5 || b.Length > 8)
{
throw new ArgumentException($"The 'b' parameter must be a string with length between {5} and {8}.", "b");
}
if (a.Length < 5 || a.Length > 8)
{
throw new ArgumentException($"The 'a' parameter must be a string with length between {5} and {8}.", "a");
}
return a + b;
}
And for outputs;
[return: StringLength(10,16)]
static string CreatePasswordValidatingOutput(string a, string b)
{
return a + b;
}
Which at compile time becomes;
[return: StringLength(10,16)]
static string CreatePasswordValidatingOutput(string a, string b)
{
string returnValue;
returnValue = a + b;
if (returnValue.Length < 10 || returnValue.Length > 16)
{
throw new PostconditionViolationException($"The return value must be a string with length between {10} and {16}.");
}
return returnValue;
}
Exactly the same contract being used in ostensibly the same way via an attribute for three quite different validation scenarios but producing consistent code at compile time that the developer has not had to write by hand.
Whilst it should be pointed out that there is a StringLength attribute that forms part of the system.ComponentModel.DataAnnotations library it does not offer the same versatility oas that provide by Metalama.Patterns.Contracts in as much as it cannot be applied to return values and there is a necessity for the developer to provide their own error message.
If you'd like to know more about Metalama in general then visit our website.
You can learn more about Metalama contracts here.
Why not join us on Slack where you can keep up with what's new and get answers to any technical questions that you might have.
Markdown/CommonTasks/ct2.md
Outdated
@@ -0,0 +1,103 @@ | |||
# Common Tasks: Meeting Requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Common Tasks: Meeting Requirements | |
# Common Tasks: Verifying required parameters and fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
Markdown/UsingMetalama/using1.md
Outdated
<br> | ||
|
||
Syntax highlighting of specific Metalama keywords is also provided by this tool which is particularly useful when creating your own custom aspects. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also talk about the aspect explorer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and CodeLens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gif does show the code lens, but an additional illustration would emphasise that. Obviously adding the Aspect Preview into this as well would make consummate sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this in the announcements on the new preview.
I had deliberately stuck to using the latest supported edition. Ergo maybe the aspect explorer would be better saved for a later email?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The email chain will be released with 2024.1 so it's fine to use any 2024.1 feature.
Markdown/UsingMetalama/using3.md
Outdated
@@ -0,0 +1,190 @@ | |||
# Using Metalama: Inheritance (via [Inheritable]) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be moved to the "creating aspects" sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually thought the same at first and then had second thoughts. I guess it boils down to this; If almost any metalama aspect could be made inheritable then I would argue that this is the right place, however if in reality it should only be used with a few specific aspect types then yes it would be more suited to to creating aspects.
Markdown/CommonTasks/ct5.md
Outdated
@@ -0,0 +1 @@ | |||
# Common Tasks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's for me to know and you to guess!!
Simply a placeholder for a new email that's all.
Markdown/CommonTasks/ct1.md
Outdated
|
||
The benefit of using Metalama to handle such tasks is that they are handled consistently whilst your own code remains concise, easy to read and comprehend. | ||
|
||
<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also say how it differs from system DataAnnotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Covered in revision added to earlier comment.
Markdown/CommonTasks/ct4.md
Outdated
|
||
If you'd like to know more about Metalama in general then visit our [website](https://www.postsharp.net/metalama). | ||
|
||
Why not join us on [Slack](https://www.postsharp.net/slack) where you can keep up with what's new and get answers to any technical questions that you might have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also need an article about validating references.
Co-authored-by: Gael Fraiteur <[email protected]>
GPT-based corrections
Bumps [json](https://github.com/ruby/json) from 2.10.1 to 2.10.2. - [Release notes](https://github.com/ruby/json/releases) - [Changelog](https://github.com/ruby/json/blob/master/CHANGES.md) - [Commits](ruby/json@v2.10.1...v2.10.2) --- updated-dependencies: - dependency-name: json dependency-version: 2.10.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
Bump json from 2.10.1 to 2.10.2
Bumps [uri](https://github.com/ruby/uri) from 1.0.2 to 1.0.3. - [Release notes](https://github.com/ruby/uri/releases) - [Commits](ruby/uri@v1.0.2...v1.0.3) --- updated-dependencies: - dependency-name: uri dependency-version: 1.0.3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.18.2 to 1.18.8. - [Release notes](https://github.com/sparklemotion/nokogiri/releases) - [Changelog](https://github.com/sparklemotion/nokogiri/blob/main/CHANGELOG.md) - [Commits](sparklemotion/nokogiri@v1.18.2...v1.18.8) --- updated-dependencies: - dependency-name: nokogiri dependency-version: 1.18.8 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
Bump uri from 1.0.2 to 1.0.3
Bump nokogiri from 1.18.2 to 1.18.8
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Hello @github-advanced-security[bot], thank you for submitting this comment. We will try to get back to you as soon as possible. |
def self.fetch(url) | ||
# Access the class-level instance variable using self.class | ||
@cache.fetch(url, expires_in: 5.minutes) do | ||
URI.open(url).read # This is executed only if the cache is missed |
Check failure
Code scanning / CodeQL
Use of `Kernel.open` or `IO.read` or similar sinks with a non-constant value Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
To fix the problem, replace the use of URI.open(url)
with a safer alternative that does not invoke Kernel.open
with a potentially user-controlled string. The recommended approach is to use URI(url).open
, which parses the URL and only opens it if it is a valid URI, avoiding the shell command execution risk. This change should be made in the CachedFetcher.fetch
method, specifically on line 15. No additional imports are needed, as the code already requires open-uri
. The functionality will remain the same, but the security risk will be mitigated.
-
Copy modified line R15
@@ -14,3 +14,3 @@ | ||
@cache.fetch(url, expires_in: 5.minutes) do | ||
URI.open(url).read # This is executed only if the cache is missed | ||
URI(url).open.read # This is executed only if the cache is missed | ||
end |
debug("Getting contents of specified remote file: #{url}") | ||
|
||
if url !~ /^https?:\/\// | ||
return URI.open(url).read |
Check failure
Code scanning / CodeQL
Use of `Kernel.open` or `IO.read` or similar sinks with a non-constant value Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
To fix the problem, replace the use of URI.open(url)
with a safer alternative that does not invoke Kernel.open
with a user-controlled string. The recommended approach is to use URI(url).open
, which parses the string as a URI and only allows supported URI schemes, or to use an HTTP client for remote resources. In this context, since the code is handling both HTTP(S) and potentially other URIs, we should replace URI.open(url)
with URI(url).open
for non-HTTP URLs. This change should be made in the get_remote_file_contents
method, specifically on line 122. No additional imports are needed, as require "open-uri"
is already present.
-
Copy modified line R122
@@ -121,3 +121,3 @@ | ||
if url !~ /^https?:\/\// | ||
return URI.open(url).read | ||
return URI(url).open.read | ||
else |
snippet_end_found = true | ||
debug("Snippet '#{snippet_name}' end matched by line: #{line}") | ||
break | ||
elsif %r!\[<(end)?snippet\s+[^>]+>\]!.match?(line) |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
regular expression
user-provided value
def remove_all_snippets(text) | ||
result_text = "" | ||
text.each_line do |line| | ||
if %r!\[<(end)?snippet\s+[^>]+>\]!.match?(line) |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
First set of e mail articles for review