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

Custom Uri Functions #378 #430

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
3 participants
@YogiBear52
Copy link

commented Dec 22, 2015

  • CustomUriFunctions - public API to add/remove custom functions or override built-In functions.
  • CustomUriFunctions and BuiltInUriFunctions tests.
  • Renamed class from 'BuiltInFunctions' to 'BuiltInUriFunctions'.
  • The new class 'UriFunctionsHelper' contains general usage for both Built-In and Custom functions.
YogiBear52
Built-In Functions to be customized #378
* CustomUriFunctions - public API to add/remove or override builtIn functions.
* CustomUriFunctions and BuiltInFunctions tests
* Changed class name from 'BuiltInFunctions' to 'BuiltInUriFunctions'.
* The new class 'UriFunctionsHelper' contains general usage for both BuiltIn and Custom functions.
@msftclas

This comment has been minimized.

Copy link

commented Dec 22, 2015

Hi @YogiBear52, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

YogiBear52 added some commits Jan 2, 2016

YogiBear52
* CustomUriFunction - Added test using ODataUriParser.
* Changed name of some methods :
'MatchSignatureToBuiltInFunction' to 'MatchSignatureToUriFunction'.
'BindAsBuiltInFunction' to 'BindAsUriFunction'.
if (!BuiltInFunctions.TryGetBuiltInFunction(functionName, out signatures))

// Try to find the function in the user custom functions
if (!CustomUriFunctions.TryGetCustomFunction(functionName, out signatures))

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

search built-in first, then custom

This comment has been minimized.

Copy link
@YogiBear52

YogiBear52 Jan 8, 2016

Author

The method 'AddCustomUriFunction' in 'CustomUriFunctions' class gets a 'overrideBuiltInFunction' parameter which indicates whether to override the existing built-in function with the same name of the given custom function.
As I mentioned in a comment: "It overrides the BuiltIn function because search is first on CustomFunctions and then on BuiltInFunctions. Another option is to remove the BuiltIn function - seems like a worse option.".
I gave the user an option to to override the built in functions with the same name, so the OData protocol stay with no changes. I could have removed the Built-In function, but it seems like the worse option because if the user will removed a custom function, which had been overrided and removed a built-in function, there will be no "true roll-back". I define a "true roll-back" as to add the removed built-in function again.
When searching first on CustomUriFunction and then on BuiltInUriFunction, adding an overriding custom function will be easy,
so when an overriding CustomUriFunction will be removed, the built-in function will always stay the same and make sure the OData protocol standstill.

  • This is why I added some tests about who is searched first.

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 11, 2016

Contributor

@YogiBear52, What i get from the code is that the AddCustomUriFunction could add a new custom function which has a different signature from all the same named built-in functions. if the signatures are same, the code will throw CustomUriFunctions_AddCustomUriFunction_BuiltInExistsFullSignature exception, right?
So i think, maybe we should return both the built-in and custom functions with the name if 'overrideBuiltInFunction' is true.

This comment has been minimized.

Copy link
@YogiBear52

YogiBear52 Jan 11, 2016

Author

So you say that we keep the 'AddCustomUriFunction' method as is, and modify the 'GetUriFunctionSignatures' method to combine the overloads of both Built-In functions and CustomFunctions. Seems like a better solution : )

Confirm that this is what you ment and I will code, document and add tests.

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 12, 2016

Contributor

@YogiBear52,It is exactly what I mean. Thanks.

public static void AddCustomUriFunction(string customFunctionName, FunctionSignatureWithReturnType newCustomFunctionSignature, bool overrideBuiltInFunction = false)
{
// Parameters validation
if (string.IsNullOrEmpty(customFunctionName))

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

Use ExceptionUtils.CheckArgumentNotNull /ExceptionUtils.CheckArgumentStringNotNullOrEmpty

same for link 67-70, line 163-166, line 263-266

}

// Check if the arguments are equal
for (int argumnetIndex = 0; argumnetIndex < functionOne.ArgumentTypes.Length; argumnetIndex++)

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

typo : argumentIndex

{
Debug.Assert(name != null, "name != null");

return CustomFunctions.TryGetValue(name, out signatures);

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

need to lock this line. TryGetValue may corrupt if another thread calls Add

CustomUriFunctions.AddCustomUriFunction(customFunctionName, existingCustomFunctionSignature);

//Test

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

delete blank lines.

string customFunctionName = "my.ExistingCustomFunction";
try
{
// Preaper

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

preaper? typo? same for some other lines

@@ -1326,6 +1256,36 @@ public void CannotCallFunctionInOpenTypeSpace()
bindOpenFunction.ShouldThrow<ODataException>().WithMessage(ODataErrorStrings.FunctionCallBinder_CallingFunctionOnOpenProperty("Fully.Qualified.Namespace.FindMyOwner"));
}

// Make sure function signatures are first searched in Custom Functions cache, and then in BuiltIn cache
[Fact]
public void GetUriFunction_SearchForCustomFunctionBeforeBuiltIn()

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

For most custom they don't use custom functions. so it should be better to search built-in first, then custom.

}
finally
{
// Clean uo CustomFunctions cache

This comment has been minimized.

Copy link
@LaylaLiu

LaylaLiu Jan 8, 2016

Contributor

uo?

YogiBear52 added some commits Jan 8, 2016

YogiBear52
Fixes to LaylaLiu, Issue #378 #430
- Usage of 'ExceptionUtils'
- Typo fixes
- lock 'TryGetCustomFunction' method
YogiBear52
- FunctionCallBinder 'GetUriFunctionSignatures' method to combine Bui…
…lt-In and Custom uri functions signatures.

- 'AddCustomUriFunction' method 'addAsOverloadToBuiltInFunction' paramater name changed from 'overrideBuiltInFunction'.

LaylaLiu added a commit to LaylaLiu/odata.net that referenced this pull request Jan 13, 2016

Fixes to LaylaLiu, Issue OData#378 OData#430
- Usage of 'ExceptionUtils'
- Typo fixes
- lock 'TryGetCustomFunction' method
@LaylaLiu

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

Have commited by e975bcd

@YogiBear52

This comment has been minimized.

Copy link
Author

commented Jan 16, 2016

This request has solved issue #378. Do you want to close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.