New Logging API Obj-C

[velicuvlad]

• CBL-6455
• old Logging API uses the new LogSink API, thanks @pasin
• do NOT allow the use new and old API at the same time.
• CBL-6460 changed/adapted existing tests to use the new API.
• renamed old tests under LogTestOld, which will be removed altogether with the old api, when time comes
• mark the old api as deprecated

[velicuvlad]

Jenkins still failing only on [MiscTest testCBLVersionAndLiteCoreVersion] ... again, is just a sanity check - will be fixed once we get a 3.2 LiteCore including couchbase/couchbase-lite-core#2198

[bmeike]

Looks pretty good. Not a close read but the parts I understand seem fine.

[velicuvlad]

ready for review now:

• • tearDown in CBLTestCase to reset vAPI at the end of each class. With this occasion, CBLLogSinks is available in any test class and can be used further on 3.2 line for testing.

• version api will be checked for every call (get/set). Added small "revers use" test at the end which expects exception.

• Jenkins will use 1.0.0 official release of VS, until I find out the next good build - 1.0.1 is failing on some tests and 2.0 throws from LiteCore. I guess 3.2.x will continue to use 1.0.x VS only
  https://github.com/couchbaselabs/couchbase-lite-ios-ee/pull/252

... testCBLVersionAndLiteCoreVersion failing only

[pasin]

I'm not quite sure that I understand the mechanism to mark and verify whether the old or the new API is used here. As discussed, iOS could follow the Java to implement the mechanism which could be done by :

• Adding a version property to each type of the LogSink. By default, the property is set to NEW. When creating the LogSink object from the old API, set the version property to OLD.
• When the LogSink object is set in the LogSinks class, perform the API version validation. If the version is NONE, record the API version.
• In the LogSinks's init method, reset the API version to NONE after setting the default console LogSink so that users can now choose which API version they want to use without getting an exception from the validation.
• In the LogSinks class, provide a reset API version method so that the tests can be used to reset the API version to None. This will allow to tests both old and new logging API.

See below for the implementation details.

CBLLogSinks+Internal.h

• A new CBLLogApiSource protocol is added with the version property that can be used to mark where the CBLXXXLogSink is created (old vs new api).

• Make CBLConsoleLogSink, CBLCustomLogSink, and CBLFileLogSink implement CBLLogApiSource protocol.

• Change CBLLogSinks's checkLogApiVersion to use id< CBLLogApiSource> as its parameter.

• Add + resetApiVersion to CBLLogSinks for the old and new logging tests to call to reset the API version.

• I have modified the current LogAPI a bit to make it inline with other enum style in the code.

typedef NS_ENUM(NSUInteger, CBLLogAPI) {
    kCBLLogAPINone,
    kCBLLogAPIOld,
    kCBLLogAPINew,
};

@protocol CBLLogApiSource <NSObject>
@property (nonatomic) CBLLogAPI version;
@end

@interface CBLConsoleLogSink () <CBLLogSinkProtocol, CBLLogApiSource>
@end

@interface CBLCustomLogSink () <CBLLogSinkProtocol, CBLLogApiSource>
@end

@interface CBLFileLogSink () <CBLLogSinkProtocol, CBLLogApiSource>
...
@end

CBLConsoleLogSink.m, CBLCustomLogSink.m, CBLFileLogSink.m

• Add @synthesize version=_version;
• In their constructor, set version to kCBLLogAPINew

@implementation CBLConsoleLogSink
...
@synthesize version=_version;

- (instancetype) initWithLevel: (CBLLogLevel)level domain: (CBLLogDomain)domain {
    self = [super init];
    if (self) {
        ...
        _version = kCBLLogAPINew;
    }
    return self;
}

CBLConsoleLogger.m and CBLFileLogger.m

• When create the CBLXXXLogSink object, set the version to kCBLLogAPIOld.
• It's OK to not doing any checking in the getter methods.
• See example below.

@implementation CBLConsoleLogger

- (void) setLevel: (CBLLogLevel)level {
    CBL_LOCK(self) {
        _level = level;
        CBLConsoleLogSink* logSink = [[CBLConsoleLogSink alloc] initWithLevel: _level  domain: _domains];
        logSink.version = kCBLLogAPIOld;
        CBLLogSinks.console = logSink;
    }
}

- (CBLLogLevel) level {
    CBL_LOCK(self) {
        return _level;
    }
}

- (void) setDomains: (CBLLogDomain)domains {
    CBL_LOCK(self) {
        _domains = domains;
        CBLConsoleLogSink* logSink = [[CBLConsoleLogSink alloc] initWithLevel: _level  domain: _domains];
        logSink.version = kCBLLogAPIOld;
        CBLLogSinks.console = logSink;
    }
}

- (CBLLogDomain) domains {
    CBL_LOCK(self) {
        return _domains;
    }
}

@end

CBLLog.m

• Do the same thing with the custom logger:

- (void) setCustom: (id<CBLLogger>)custom {
    CBL_LOCK(self) {
        _custom = custom;
        CBLCustomLogSinkBridge* bridge = [[CBLCustomLogSinkBridge alloc] initWithLogger: _custom];
        CBLCustomLogSink* customSink = [[CBLCustomLogSink alloc] initWithLevel: _custom.level logSink: bridge];
        customSink.version = kCBLLogAPIOld;
        CBLLogSinks.custom = customSink;
    }
}

- (id<CBLLogger>) custom {
    CBL_LOCK(self) {
        return _custom;
    }
}

CBLLogSinks.mm

• In the + (void)init method, reset _vAPI to kCBLLogAPINone after setting the default console logger.
• Change checkLogApiVersion: to take id as its parameter as follows.
• Add resetApiVersion implementation.
• For setConsole:, setCustom:, setFile:, call [self checkLogApiVersion:] to verify the version.

+ (void) init {
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        ...
        self.console = [[CBLConsoleLogSink alloc] initWithLevel: kCBLLogLevelWarning];
        
        // Reset the API version
        [self resetApiVersion];
    });
}

+ (void) setConsole:(CBLConsoleLogSink*)console {
    CBL_LOCK(self) {
        [self checkLogApiVersion: console];
        _console = console;
    }
    [self updateLogLevels];
}

+ (void) setCustom: (CBLCustomLogSink*) custom {
    CBL_LOCK(self) {
        [self checkLogApiVersion: custom];
        _custom = custom;
    }
    [self updateLogLevels];
}

+ (void) setFile: (CBLFileLogSink*) file {
    CBL_LOCK(self) {
        [self checkLogApiVersion: file];
        _file = file;
    }
    [CBLFileLogSink setup: file];
    [self updateLogLevels];
}

#pragma mark - Internal

+ (void) checkLogApiVersion: (id<CBLLogApiSource>)source {
    if (_vAPI == kCBLLogAPINone) {
        _vAPI = source.version;
    } else if (_vAPI != source.version) {
        [NSException raise: NSInternalInconsistencyException
                    format: @"Cannot use both new and old Logging API simultaneously."];
    }
}

+ (void) resetApiVersion {
    _vAPI = kCBLLogAPINone;
}

When setting up each login test (old and new api), call + (void) resetApiVersion to reset the API version.

[velicuvlad]

Having the version inside the sinks themselves doesn't work. It will crash whenever one of them is set to nil and we allow them to be nil. I also don't understand how this checks enforce one another (old and new logging). To me it seems that only the new api checks for the old one and not the other way around as well.

Those LogApi version grabs whatever the current version is CBLLogSinks under. And it matters if is None or Old. And just sets it back. So in the case Old is being called and Old has been used previously, we want to keep that (guard against a new, new api, call). And CBLLogSinks control None value as well, so it will only happen when set to LogApiNone in the internal implementation. It will never go to None from an user API call perspective.

[pasin]

For the nil case, the checkLogApiVersion: can just be no-ops, I think. It's not 100% perfect but it will serve the purpose enough that we don't want users to use two API to setup the log at the same time. Otherwise, you will need to add another layer for the implementation to allow the perfect check. Or needs to have something like an internal CBLNullConsoleLockSink that the old API can use which is a bit too much.

+ (void) checkLogApiVersion: (id<CBLLogApiSource>)source {
    if (!source) {
      return;
    }
    ...
}

Update:

The old API implementation has never set the log sink to nil so this can be handled as this instead :

+ (void) checkLogApiVersion: (id<CBLLogApiSource>)source {
    // The old API will not set the source to nil so when the source is nil, 
    // assume that it's from the new API.
    CBLLogAPI veresion = source ? souce.version :  kCBLLogAPINew;
     if (_vAPI == kCBLLogAPINone) { 
        _vAPI = version;
    } else if (_vAPI != version) { 
        [NSException raise: NSInternalInconsistencyException format: @"Cannot use both new and old Logging API simultaneously."];
     } 
}

[pasin]

Those LogApi version grabs whatever the current version is CBLLogSinks under. And it matters if is None or Old. And just sets it back. So in the case Old is being called and Old has been used previously, we want to keep that (guard against a new, new api, call). And CBLLogSinks control None value as well, so it will only happen when set to LogApiNone in the internal implementation. It will never go to None from a user API call perspective.

I don't understand this point. The None is the status before one of the API is used. When the old API is used, the + (void) checkLogApiVersion: (id<CBLLogApiSource>)source will keep the value as OLD so that the old API can be continue to be used.

[velicuvlad]

Another reason why I don't really want, although I tried, is that each individual sink doesn't even have to know about the api version. From the implementation view, only CBLLogSinks is the manager. It can change its variable based on what receives on a individual sink init. As CBLLogSinks will be init by the time underlying sinks are created.

Indeed, for a "perfect" implementation it should also check if the call above is received from CBLLog/db.log... but I couldn't manage that. And so, I bypass each call of updateSinks inside CBLLog, ConsoleLogger, FileLogger to allow for the call (set) and revert back to whatever value was received. And it will only happen when flag is None which is only right after init or Old, which means another old api call was made beforehand.

If the flag received is New, it means there was 1 call of the new API, after init. And so this then will throw due to the check on the old api.

i.e ConsoleLogger

- (void) setDomains: (CBLLogDomain)domains {
    LogAPI version;
    CBL_LOCK(self) {
        version = [CBLLogSinks vAPI];
        [CBLLogSinks checkLogApiVersion: LogAPIOld];
        _domains = domains;
    }
    [self updateConsoleLogSink];
    [CBLLogSinks setVAPI: version];
}

- (CBLLogDomain) domains {
    CBL_LOCK(self) {
        [CBLLogSinks checkLogApiVersion: LogAPIOld];
        return _domains;
    }
}

[pasin]

From my understanding, this will not work because when setting log sink at the new API, it will set the API to new (Actually, it works now because the code to mark that the new API is used is MISSING. Once adding that, you can see that this logic doesn't work). Also can you clarify why the version needs to be gotten first and set it back.

[velicuvlad]

The code that marks that NEW Api is used is when checking for NEW with [CBLLogSinks checkLogApiVersion: LogAPINew]; in the initWithLevel of every NEW api sink. The first call of that will set it to NEW. the only way that does NOT throw is when vAPI was previously None (during init or right after init) or New.

1. On the OLD api, the current version needs to be gotten first because we need to know if it was Old or None (at the old api level set/get). When it is Old, we change it to new to allow mem allocation for a new sink (the one in updateXLogSink - see above), then we revert to Old (so we guard against future NEW api calls). If it is None, it basically means it is somewhere during init, hence we set it back to None.

2. vAPI is dynamically assigned on checkLogApiVersion call to Old (set/get for old api) or New (and initWithLevel for new api log sinks) whenever that it is None. So in order to allow updateXLogSink calls we need to capture it beforehand to check if that was just set from the call itself, meaning it was None before, or was it already Old. We are mainly interested into keeping it if it was Old, as that will further guard for NEW api call down the line. For New will, of course, throw right away.

[velicuvlad]

    // The old API will not set the source to nil so when the source is nil, 
    // assume that it's from the new API.
    CBLLogAPI veresion = source ? souce.version :  kCBLLogAPINew;
     if (_vAPI == kCBLLogAPINone) { 
        _vAPI = version;
    } else if (_vAPI != version) { 
        [NSException raise: NSInternalInconsistencyException format: @"Cannot use both new and old Logging API simultaneously."];
     } 
}

The old one does set nil, at least for the file logger. if config is nil during setLevel or setConfig then CBLLogSinks.file should be nil.

[velicuvlad]

Will take Blake's way of handling this case that was discussed in the meeting. Seems it does actually init a FileLogSink with level none and some other set attr which is sending to LiteCore, instead of invalidating the whole sink (null).

[velicuvlad]

[[CBLFileLogSink alloc] initWithLevel: kCBLLogLevelNone
                            directory: @""
                         usePlaintext: false
                         maxKeptFiles: 0
                          maxFileSize: 0
                      ];

[velicuvlad]

CBLLog init is first creating the new logging logic and then creating the old one. During the old one init, it itself calls to update the new one again. It cannot be the other way around. We've made old one use the implementation of the old one logic wise, but the usage of it is, in fact, is the other way. The old now can't do anything without the new one existing because it updates it in underlying implementation.

I couldn't find a way to reset that only once and I've tried...

[bmeike]

Yeah... address Pasin's comments and it will be fine. My apologies for handing you such a PITA.

[pasin]

I have some format and test related comments. Please take a look. The rest look fine. Once the swift API is done, let come back and get another look for the number types that you have raised before.

[pasin]

What is the case that this function is called with path == nil? If there is a case, maybe it's more straightforward to just assert the path in that test as it doesn't seem like it's responsibility of this method to verify the path?

[velicuvlad]

I will remove this, alongside test testFileLoggingDisabled because it is redundant. The only check we can do is that .directory is nil when the .file is nil... which is always true. We already set it to nil in the next test testFileLoggingReEnableLogging so I will move the assert for .directory there.

[velicuvlad]

I will keep it for the old test because there we do set level None, but we actually create a file log

[pasin]

This may not work when we want to debug an issue by enable console log in the setup method. Probably move to the regular setup method in this class?

[velicuvlad]

That is true if you enable console logging at the CBLTestCase level setup method.

I moved it under -setUp and I have added one in -tearDown as well. It is needed in case there's additional logging configuration made in test classes. For now is only in the log tests classes, but it might be the case in the future.

Tested with enabling verbose logging using new api, before reset and it work. Marked with a comment.