[SERVER-17349] Source Code refactor - Abstract common functionality Created: 23/Feb/15  Updated: 17/Mar/15  Resolved: 09/Mar/15

Status: Closed
Project: Core Server
Component/s: Security
Affects Version/s: None
Fix Version/s: 3.1.0

Type: Improvement Priority: Major - P3
Reporter: Krishna Narasimhan Assignee: Ramon Fernandez Marina
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Participants:

 Comments   
Comment by Ramon Fernandez Marina [ 09/Mar/15 ]

I forgot to add ticket numbers in the revision history – my apologies. Here are the relevant commits:

commit efd11b1106a9082908b6c098c397b509f66c75b4
Author: Krishna Narasimhan <krishna.nm86@gmail.com>
Date:   Mon Feb 23 15:09:11 2015 +0100
 
    Corrected issue. Paramatrized one more constant in the merged method
    
    Closes #928
    
    Signed-off-by: Ramon Fernandez <ramon@mongodb.com>
 
commit a4052b7cf11ac3ae0a67fc78275f4eaa5942e5cb
Author: Krishna Narasimhan <krishna.nm86@gmail.com>
Date:   Mon Feb 23 15:06:01 2015 +0100
 
    Source code refactor. Removed redundant functionalities in user_managements_command
    
    Part of #928
    
    Signed-off-by: Ramon Fernandez <ramon@mongodb.com>
 
commit 59e53003bda91156cf70db85f94a6274e012398c
Author: Krishna <krishna.nm86@gmail.com>
Date:   Wed Feb 25 10:02:59 2015 +0100
 
    Introduced required function declaration in header
    
    Closes #927
    
    Signed-off-by: Ramon Fernandez <ramon@mongodb.com>
 
commit 283187b470ba97a6197effb5f17f16b85dae1d47
Author: Krishna Narasimhan <krishna.nm86@gmail.com>
Date:   Mon Feb 23 14:48:33 2015 +0100
 
    Source refactor. Removed redundant functionality in db/auth/authorization_sessions
    
    Part of #927
    
    Signed-off-by: Ramon Fernandez <ramon@mongodb.com>

krishna.nm86, thanks for your contributions. I'd like to ask to please follow these guidelines for future pull requests:

  • Keep all related changes in one commit (see "git merge --squash" for details). Having just one commit makes it a lot easier to merge the request
  • Add the relevant SERVER ticket to the revision history. That way your commits get logged in the right ticket even if I forget to add the ticket
  • Trailing whitespace is considered "bad thing" – please make sure you eliminate any trailing whitespace (as in #928) in the future to facilitate merges (I use "git am -s --whitespace=error" to apply patches)

I'm going to resolve this ticket and its subtasks. Having a sub-task per pull request is a bit of an overkill, and if all pull requests are in one commit opening one ticket is enough

Thanks again,
Ramón.

Comment by Krishna Narasimhan [ 06/Mar/15 ]

If the test worked, I could make the changes and update the pull request

Comment by Krishna Narasimhan [ 05/Mar/15 ]

Thanks a lot. Would be an exciting result if the approach could have results mergeable by good quality repositories.

Comment by Ramon Fernandez Marina [ 05/Mar/15 ]

Found a simple typo in #927, where ";" and ")" need to be exchanged:

diff --git a/src/mongo/db/auth/authorization_session.h b/src/mongo/db/auth/authorization_session.h
index 190b854..1085594 100644
--- a/src/mongo/db/auth/authorization_session.h
+++ b/src/mongo/db/auth/authorization_session.h
@@ -156,7 +156,10 @@ namespace mongo {
         // Utility function for isAuthorizedForActionsOnResource(
         //         ResourcePattern::forDatabaseName(role.getDB()), ActionType::grantAnyRole)
         bool isAuthorizedToRevokeRole(const RoleName& role);
-
+
+        // Utility function for isAuthorizedToChangeOwnPasswordAsUser and isAuthorizedToChangeOwnCustomDataAsUser
+        bool isAuthorizedToChangeAsUser(const UserName& userName, ActionType actionType;)
+                                                                                       ^^
         // Returns true if the current session is authenticated as the given user and that user
         // is allowed to change his/her own password
         bool isAuthorizedToChangeOwnPasswordAsUser(const UserName& userName);

There's also some trailing whitespace which is flagged by style checkers. I'm manually fixing those two things and running the result through our testing.

Comment by Krishna Narasimhan [ 05/Mar/15 ]

Hi,
What exactly is the error you are getting with #927? Could you also tell me what happens with #928.

I am using an Eclipse plugin which I wrote myself that identifies potentially copy-pasted code and abstracts them. Of course, the ultimate decision is upto the maintainers to accept a pull request if it makes sense in the long run for them. These 2 particular pull requests were part of a prototype evaluation of my tool , in order to decide further working based on it with the percentage of accepted merges. Especially #928 seems to have saved a lot of redundant code space and reduced code duplication , so would be nice to hear if one orb oth of these could be deemed acceptable by your code standards.

Thanks,
Krishna

Comment by Ramon Fernandez Marina [ 05/Mar/15 ]

Hi krishna.nm86,

I'm not able to compile #927 as it is, haven't tried #928 yet.

Can you please provide some more details on the refactoring software you're using and the context of your work? It would be useful to know a bit more about the bigger picture: while abstracting common functionality is in principle a good thing, some of your pull requests may affect other/larger refactoring/rewriting plans so it may not be possible/convenient for us to accept them at this stage.

Thanks,
Ramón.

Comment by Krishna Narasimhan [ 04/Mar/15 ]

Thanks a lot. Actually this is part of the evaluation of Programming Re factoring research approach and would be really nice to get feedback or successful merges from high quality code bases like yours.

Regards
Krishna

Comment by Andreas Nilsson [ 03/Mar/15 ]

krishna.nm86 we have been a little caught up with the 3.0 release, sorry for the delay in getting back to you.

I am reassigning the ticket to ramon.fernandez who handles our external pull requests.

Thank you,
Andreas

Comment by Krishna Narasimhan [ 03/Mar/15 ]

Hi @Andreas Nilsson,
Any news on the status of my pull requests?

Comment by Krishna Narasimhan [ 27/Feb/15 ]

I would like to merge these two pull requests

1.https://github.com/mongodb/mongo/pull/928

2.https://github.com/mongodb/mongo/pull/927

Comment by Andreas Nilsson [ 27/Feb/15 ]

Hi krishna.nm86 can you elaborate on what you are looking to do with these tickets.

Do you have a pull request you would like to merge or are you suggesting changes/bug fixes to the server?

Best regards,
Andreas

Comment by Krishna Narasimhan [ 25/Feb/15 ]

Thanks . I have made one small fix to the change I made. Hope that helps.

Comment by Ramon Fernandez Marina [ 24/Feb/15 ]

krishna.nm86, when a ticket has the "Needs Triage" fixversion the ball is in our court to decide if a ticket will be part of the server, and if so, where does it fit in the planning. Bottom line, no action item on your end as of now.

Comment by Krishna Narasimhan [ 24/Feb/15 ]

I am unfamiliar with the terminology here. What does needs triage mean. Is there something i need to do now. Or just wait for feedback

Comment by Krishna Narasimhan [ 23/Feb/15 ]

Hi Andrew.
Thanks a lot for the response. This is my first time contributing for Mongodb and I am excited. So thanks for the response. You can move it to the right project and I greatly appreciate that help.

The rationale behind this refactor is to reduce the amount of redundant code and using Abstraction practices to keep in sync with the Software Engineering principle of "Donot Repeat Yourself (DRY) "

Comment by Andrew Morrow (Inactive) [ 23/Feb/15 ]

krishna.nm86 -

This project is for the C language MongoDB driver, but the functions you identify above are part of the MongoDB server, mongod. The correct project for issues with the server is the SERVER project. Can I refile these tickets for you in that project?

Additionally, you should provide some motivation or rationale for the changes that you are suggesting.

Thanks,
Andrew

Generated at Thu Feb 08 03:44:07 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.